Page MenuHomePhabricator

HTML previews' layout breaks text multi-line text truncation
Closed, ResolvedPublic

Description

The new version of page previews has a number of issues, including:

  • extracts displaying multiple paragraphs Out of scope for this task.
  • Horizontal gradient not quite overlapping text properly.
  • Text cut off at bottom of extract.

Steps to reproduce

Text cut off

  1. Go to: https://en.wikipedia.beta.wmflabs.org/wiki/Birdman_(film)
  2. Hover over Alejandro G. Iñárritu

Expected behavior: last line of text is not cut off/does not appear

Horizontal gradient overlapping text

  1. Go to: https://en.wikipedia.beta.wmflabs.org/wiki/Category:Living_people
  2. Hover over “Andrew Miller”

Expected behavior: Horizontal gradient does not appear
Observed behavior: top of horizontal gradient overlaps text

Both

  1. Go to: https://en.wikipedia.beta.wmflabs.org/wiki/Category:Articles_with_%27species%27_microformats
  2. Hover over “european mink”

Expected behavior: paragraphs should be concatenated

Details

Related Gerrit Patches:
mediawiki/extensions/Popups : masterRemove paragraph margins from HTML extracts

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 19 2017, 6:24 PM
ovasileva triaged this task as High priority.
ovasileva added subscribers: phuedx, pmiazga.
phuedx updated the task description. (Show Details)EditedJun 20 2017, 5:34 PM

This feels like a styling issue caused by the addition of the p wrapper element to the container element, i.e. it's altering the size of the container element. I suspect that if we reset the styles of the wrapper element, then both issues will disappear.

bmansurov added a subscriber: Nirzar.
Jdlrobson added a subscriber: Jdlrobson.EditedJun 20 2017, 7:24 PM

@ovasileva I should remind everyone that the purpose of T165018 was to make it possible to show HTML - not to fix all the problems. That's why it was estimated at 3 points.

It worries me about the scope creep here. Given the need for design, should this really be in the current sprint? Personally I would like us to slow down, identify the problems in preparation for next sprint. There is a risk associated with adding unplanned work that has not been discussed by all the team.

@Jdlrobson - we discussed this during standup yesterday and today - looks like there might be a fix for this issue based on looking at line height that doesn't need any changes to design. In terms of planned/unplanned - we mentioned a few times during prioritization and planning that we will be including some of the bugs caused by T165018: Page previews can consume new summary-HTML endpoint into this sprint. The remainder of the bugs though aren't ready for work yet and are in needs analysis - most likely for next sprint.

ovasileva updated the task description. (Show Details)Jun 20 2017, 7:49 PM

@Jdlrobson - we discussed this during standup yesterday and today - looks like there might be a fix for this issue based on looking at line height that doesn't need any changes to design. In terms of planned/unplanned - we mentioned a few times during prioritization and planning that we will be including some of the bugs caused by T165018: Page previews can consume new summary-HTML endpoint into this sprint. The remainder of the bugs though aren't ready for work yet and are in needs analysis - most likely for next sprint.

Sure, but this is not a subtask of T16501 IMO and should be tagged unplanned sprint work. We did not plan to do this. This is a new bug that has been noticed as a side effect of enabling the summary endpoint and by pulling it in we are adding risk so should reflect this.

@Nirzar, as @phuedx says in T168332#3364440, the problem is arising from us setting a predefined height, but p elements having margins. Should we remove margins? If so, the problem will go away, but the paragraphs won't be visually separate, they'll read like one paragraph. I suppose this is fine as the extract is only a small bit of text.

Change 360415 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] Remove paragraph margins from HTML extracts

https://gerrit.wikimedia.org/r/360415

@Nirzar, as @phuedx says in T168332#3364440, the problem is arising from us setting a predefined height, but p elements having margins. Should we remove margins? If so, the problem will go away, but the paragraphs won't be visually separate, they'll read like one paragraph. I suppose this is fine as the extract is only a small bit of text.

@bmansurov - this seems like it would be close to the behavior we had before, or would there be other visual differences? If no, this is okay and we can discuss on restricting the extract to the first paragraph only later.

@ovasileva, the only difference will be the paragraphs won't be visually separated. I'm not sure of the old behavior, but I think this is an acceptable solution to the problem.

@bmansurov - the old behavior was the screenshot attached. Basically it would look like the image above? If so, yes, that's acceptable.

Oh, my bad. Yes, it will look like the old image.

phuedx added a comment.EditedJun 21 2017, 3:50 PM

Sure, but this is not a subtask of T16501 IMO…

You're absolutely right. It's definitely not a subtask of T16501: A way to hide the sidebar providing a "fullscreen" view ;)

and should be tagged unplanned sprint work. We did not plan to do this. This is a new bug that has been noticed as a side effect of enabling the summary endpoint and by pulling it in we are adding risk so should reflect this.

I think that this is a balanced approach to – loosely speaking – having an alpha version of HTML-driven previews up and running. We've noticed a minor regression – prior to QA even! – in the layout of previews caused by inserting HTML into 'em and we can fix it relatively quickly. This is by no means "all of the problems".

FTR @ovasileva, @pmiazga, and I all agreed that this is the only bug or task that should be brought into the sprint as a subtask of this task.

Change 360415 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Remove paragraph margins from HTML extracts

https://gerrit.wikimedia.org/r/360415

Here's what I see in the local environment when fetching previews from the RESTBase instance on the Beta Cluster 👍


I punted on trying to fix the class of layout bugs that occur when we talk about previews with multiple paragraphs. IMO an extract with multiple paragraphs seems… odd to me. I think we should discuss this as a team – including trying to find edge cases for whatever we land on – before taking this further.

phuedx renamed this task from HTML previews have multiple style issues to HTML previews' layout breaks text multi-line text truncation.Jun 22 2017, 9:49 AM
phuedx removed a project: Patch-For-Review.
phuedx reassigned this task from bmansurov to Jdlrobson.Jun 22 2017, 10:15 AM
phuedx added a subscriber: bmansurov.
phuedx reassigned this task from Jdlrobson to ovasileva.Jun 22 2017, 10:28 AM

@bmansurov, @phuedx - found an edge case:

  1. Go to: https://en.wikipedia.beta.wmflabs.org/wiki/Category:Living_people
  2. Hover over: Bill and Ruth Lucas

We should be stripping the disambiguation

@phuedx - related, although not a part of this task. What is the difference between the extract above (for Bill and Ruth Lucas) and the following:

  1. Go to https://en.wikipedia.beta.wmflabs.org/wiki/Category:Living_people
  2. Hover over Cryme Time

Here, we're seeing just the disambiguation, in the former, we're seeing the disambiguation and the first sentence.

@phuedx - related, although not a part of this task. What is the difference between the extract above (for Bill and Ruth Lucas) and the following:

The former (Bill and Ruth Lucas) has that text marked up as a list entry whereas the latter doesn't.

@ovasileva: In both cases, those sentences aren't marked up any meaningful way. Indeed, they aren't even marked up as hatnotes, which they are!

I think we're aware that including lists in page summaries is going to require a lot of additional work. I'm going to say that the first case is way, way out of scope here.

ovasileva added a comment.EditedJun 22 2017, 2:50 PM

yup :(

. I agree that both are out of scope though

Jdlrobson added a comment.EditedJun 22 2017, 4:56 PM

I guess this relates to how we define "unplanned sprint work". To me this is unplanned and this might not seem important but I think it helps highlight "risky" tasks in a sprint. We did not plan to do this. This is a new bug that has been noticed as a side effect of enabling the summary endpoint and by pulling it in we are adding risk so should reflect this. I'm saying we should at least acknowledge it's unplanned. Anyway, continue as you are, maybe something for retro.

@bmansurov, @phuedx - found an edge case:

  1. Go to: https://en.wikipedia.beta.wmflabs.org/wiki/Category:Living_people
  2. Hover over: Bill and Ruth Lucas

We should be stripping the disambiguation

Yep definitely an editoral issue. To demonstrate how an editor would fix such an issue, I fixed it on wiki the 2 ways that are possible: https://en.wikipedia.org/w/index.php?title=Bill_and_Ruth_Lucas&type=revision&diff=786966027&oldid=747456605 AND/OR https://en.wikipedia.org/w/index.php?title=Bill_and_Ruth_Lucas&type=revision&diff=786966284&oldid=786966027

@phuedx - related, although not a part of this task. What is the difference between the extract above (for Bill and Ruth Lucas) and the following:

  1. Go to https://en.wikipedia.beta.wmflabs.org/wiki/Category:Living_people
  2. Hover over Cryme Time


Here, we're seeing just the disambiguation, in the former, we're seeing the disambiguation and the first sentence.

This has already been fixed in production - a reminder on why it's not always helpful to test on the beta cluster which uses outdated articles.

@Jdlrobson, @phuedx - sorry, I don't think I made my original question clear enough. I was wondering why the extract for Cryme Time is shorter/contains only one paragraph, while the extract for Bill and Ruth Lucas contains more. Infobox position maybe? Something else?

@Jdlrobson, @phuedx - sorry, I don't think I made my original question clear enough. I was wondering why the extract for Cryme Time is shorter/contains only one paragraph, while the extract for Bill and Ruth Lucas contains more. Infobox position maybe? Something else?

My previous explanation – that there was a section boundary marker present after the non-hatnote hatnote – was wrong. This is a symptom of T168329: exsentences does not work correctly when HTML output used.

ovasileva closed this task as Resolved.Jun 23 2017, 10:21 AM
ovasileva reopened this task as Open.Jun 26 2017, 7:13 PM

Re-opening to include fix for lists. Spoke with @Nirzar and an accetable solution for lists would be to remove the spacing between the end of the first paragraph and the beginning of the lead, similar to T168332#3369896

ovasileva closed this task as Resolved.Jun 26 2017, 7:18 PM

never mind. I'll open a new task.