Closed Bug 1418472 Opened 7 years ago Closed 5 years ago

abs-pos placeholder may end up on the wrong side of a line-break [was: Wrong position of github issue name on hover]

Categories

(Core :: Layout, defect, P3)

All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla69
Webcompat Priority ?
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox69 --- fixed

People

(Reporter: sviter33, Assigned: jfkthame)

References

Details

(Whiteboard: [webcompat][wptsync upstream])

Attachments

(6 files)

Attached image wrong position.png
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20171117100127

Steps to reproduce:

1. Go to https://github.com/servo/servo/issues/19268
2. Hover mouse cursor over issue number on first comment


Actual results:

Name of issue is wrong positioned
I can't reproduce the issue with the latest Nightly 59.0a1 (2017-11-17) for macOS. Please troubleshoot with Safe Mode: https://support.mozilla.org/kb/troubleshoot-firefox-issues-using-safe-mode
With Safe Mode I still can see this issue. May it be platform specific or depends on screen resolution?
Can you please use https://mozilla.github.io/mozregression/ to determine the regression range?
I can reproduce this on latest nightly Fx 59.0a1, build Id 20170917100334 on Windows 10, Ubuntu 16.04 and Mac OS X 10.12. Please note that the bug is reproducible only if the link with the issue number is the first thing on the line (and not on the first line of the comment). 
On Ubuntu, I reproduce the issue with a zoom of 120%. 
On Mac, I cannot reproduce with the link from the bug description, but it is reproducible with the link: https://github.com/servo/servo/pull/13418 - search for issue #9782 and hover the mouse cursor over it.

This bug is reproducible on release 57.0 and also on Beta 58.
Component: Untriaged → Layout
Product: Firefox → Core
Version: unspecified → Trunk
I find that in 2015-10-29 Nightly build name of the issue first appear on hover. Before this build nothing happened on hover. And in 2015-10-29 build I can see this bug already.
Jet, I think we need an owner on this. Possibly some sort of internal js metric that isn't returning valid data.
Flags: needinfo?(bugs)
OS: Unspecified → Windows
Priority: -- → P2
Hardware: Unspecified → All
Appears to occur when a line break moves the link to a second line. The offset to the start of the link text is reported as the end of the previous line. Jonathan: where do we compute that? Thx!
Flags: needinfo?(bugs) → needinfo?(jfkthame)
This reminds me somewhat of bug 1418309, which also shows up on those Github links, though it's not the exact same issue.

I think the root of the issue here is that the "anchor" for the tooltip-like display of the Github issue title is inserted as a ::before pseudo-element, and placed using position:absolute, so it's an out-of-flow element. The placeholder frame then stays at the end of the preceding line, as it occupies no space and so doesn't cause the line to overflow; and then the "tooltip" gets anchored to that position.

We should probably be ensuring that placeholders don't get separated from adjacent in-flow frames during line-breaking, so that the placeholder for ::before here goes to the next line along with the link.

I'm not sure whether a fix for bug 1283222 will necessarily fix this, but they're both about how placeholder frames are handled during line-breaking, so perhaps Jeremy can look into this as well, or as a followup.
Flags: needinfo?(jfkthame) → needinfo?(jeremychen)
Hover over the link-like spans, and observe the position of the "fake" tooltips.

BTW, the second part of the testcase, using position:relative, shows similar "bad" behavior in Safari and Chrome as well; but the first part with position:absolute is OK in those browsers, and I think that's the one that's more parallel to the Github case.
(In reply to Kohei Yoshino [:kohei] from comment #3)
> Can you please use https://mozilla.github.io/mozregression/ to determine the
> regression range?

I am able to reproduce the bug on windows and Linux (latest nightly). Any github page is OK, as long as you zoom in or out so that the issue # is the first character of a new line.

I tried to do a mozregression tonight on this bug to showcase mozregression during a nightly workshop. We went back as far as 2012 and it did not work. In 2010, there was a visual glitch (blink) and before there was no tooltip.

We concluded that it never actually worked but nobody reported it (or noticed) it before today.
(In reply to Jonathan Kew (:jfkthame) from comment #8)
> This reminds me somewhat of bug 1418309, which also shows up on those Github
> links, though it's not the exact same issue.
> 
> I think the root of the issue here is that the "anchor" for the tooltip-like
> display of the Github issue title is inserted as a ::before pseudo-element,
> and placed using position:absolute, so it's an out-of-flow element. The
> placeholder frame then stays at the end of the preceding line, as it
> occupies no space and so doesn't cause the line to overflow; and then the
> "tooltip" gets anchored to that position.
> 
> We should probably be ensuring that placeholders don't get separated from
> adjacent in-flow frames during line-breaking, so that the placeholder for
> ::before here goes to the next line along with the link.
> 
> I'm not sure whether a fix for bug 1283222 will necessarily fix this, but
> they're both about how placeholder frames are handled during line-breaking,
> so perhaps Jeremy can look into this as well, or as a followup.

Hmmm... I'm not sure if I could fix this together in bug 1283222. But as you said, I agree they're somehow related, and keep this in mind while trying to fix bug 1283222 is the right thing to do.

I can look into this (maybe after bug 1283222), assign to myself for now.
Assignee: nobody → jeremychen
Flags: needinfo?(jeremychen)
FWIW, when hovering over the "problem" Github links with a debug build, we hit a warning that I believe relates to the misplaced out-of-flow element:

WARNING: Out-of-flow frame got reflowed before its placeholder: file /Users/jkew/mozdev/mozilla-central/layout/generic/nsPlaceholderFrame.cpp, line 143

And then when the mouse is moved away and the issue title is hidden again, we hit an assertion:

###!!! ASSERTION: Placeholder relationship should have been torn down already; this might mean we have a stray placeholder in the tree.: '!placeholder || nsLayoutUtils::IsProperAncestorFrame(aDestructRoot, placeholder)', file /Users/jkew/mozdev/mozilla-central/layout/generic/nsFrame.cpp, line 771

These don't occur for the "normal" case where the link is not immediately following a line-break, and the issue title is correctly placed.
Here's a frame dump of the relevant paragraph, from the example on macOS mentioned in comment 4:

Block(p)(1)@12b82a7d8 parent=12b82d0d0 next=12b491cb8 {900,900,40080,8962} vis-overflow=-30,0,40215,8962 scr-overflow=0,0,40080,8962 [state=0202120800101211] [content=12b341c10] [sc=130e0cbc8]<
  line 12b88a850: count=3 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x120] {0,0,39722,1260} vis-overflow=-30,0,40019,1260 scr-overflow=0,0,39722,1260 <
    Text(0)"First, "@12b887c10 parent=12b82a7d8 next=12b847f58 {0,120,2169,1020} vis-overflow=-30,0,2229,1020 scr-overflow=0,0,2169,1020 [state=02000048b0200000] [content=12b344880] [sc=12fe0f788:-moz-text] [run=130917310][0,7,T] 
    Inline(a)(1)@12b847f58 parent=12b82a7d8 next=12b887db0 {2169,120,2264,1020} vis-overflow=-30,0,2324,1020 scr-overflow=0,0,2264,1020 [state=0200100800000200] [content=12b33bea0] [sc=130e16468]<
      Text(0)"@jdm"@12b887d28 parent=12b847f58 {0,0,2264,1020} vis-overflow=-30,0,2324,1020 [state=02000048a0000000] [content=12b344900] [sc=12fe85bd8:-moz-text] [run=130bed900][0,4,T] 
    >
    Text(2)" sorry for letting you review a wip work. Some cha"@12b887db0 parent=12b82a7d8 next=12b9fc0c0 next-in-flow=12b9fc0c0 {4433,120,35289,1020} vis-overflow=-30,0,35586,1020 scr-overflow=0,0,35289,1020 [state=02000048a1400000] [content=12b344980] [sc=12fe0f788:-moz-text] [run=1309afc00][0,93,F] 
  >
  line 12a8fb988: count=3 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x120] {0,1260,37493,1331} vis-overflow=-30,1260,37790,1331 scr-overflow=0,1260,37493,1331 <
    Text(2)"invariant after "@12b9fc0c0 parent=12b82a7d8 next=12b848070 prev-in-flow=12b887db0 {0,1380,5583,1020} vis-overflow=-30,0,5643,1020 scr-overflow=0,0,5583,1020 [state=0200004890200004] [content=12b344980] [sc=12fe0f788:-moz-text] [run=1309afc00][93,16,T] 
    Inline(code)(3)@12b848070 parent=12b82a7d8 next=12b887f50 {5583,1537,7646,1036} [state=0200104000000200] [content=12b341ca0] [sc=130e168a8]<
      Text(0)"location.hash = """@12b887e38 parent=12b848070 {286,143,7074,750} vis-overflow=-30,0,7134,750 scr-overflow=0,0,7074,750 [state=02000048a0000000] [content=12b344a00] [sc=12fe85ce8:-moz-text] [run=130655660][0,18,T] 
    >
    Text(4)", which proves to be not enough for our test. I wi"@12b887f50 parent=12b82a7d8 next=12b9fc230 next-in-flow=12b9fc230 {13229,1380,24264,1020} vis-overflow=-30,0,24561,1020 scr-overflow=0,0,24264,1020 [state=02000048a1400000] [content=12b344a80] [sc=12fe0f788:-moz-text] [run=12aae46a0][0,66,F] 
  >
  line 12a8fb9d8: count=2 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:linebr[0x80100] {0,2591,9756,1260} vis-overflow=-30,2591,9815,1260 scr-overflow=0,2591,9756,1260 <
    Text(4)"master and clean this up."@12b9fc230 parent=12b82a7d8 next=12b8145e8 prev-in-flow=12b887f50 {0,2711,9755,1020} vis-overflow=-30,0,9815,1020 scr-overflow=0,0,9755,1020 [state=0201004890600004] [content=12b344a80] [sc=12fe0f788:-moz-text] [run=12aae46a0][66,25,T] 
    Frame(br)(5)@12b8145e8 parent=12b82a7d8 next=12b88a0b0 {9755,2591,1,1260} [state=0200000800000000] [content=12b341d30] [sc=130e16ce8]
  >
  line 12b88a8a0: count=4 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x120] {0,3851,37607,1331} vis-overflow=-30,1961,52789,3221 scr-overflow=0,1961,52759,3221 <
    Text(6)"\nAnother topic for this PR is there are some conc"@12b88a0b0 parent=12b82a7d8 next=12b848298 {0,3971,26015,1020} vis-overflow=-30,0,26075,1020 scr-overflow=0,0,26015,1020 [state=02010048b0200000] [content=12b344b00] [sc=12fe0f788:-moz-text] [run=129b27580][0,67,T] 
    Inline(code)(7)@12b848298 parent=12b82a7d8 next=12b88a250 {26015,4128,5288,1036} [state=0202104000000200] [content=12b341dc0] [sc=130e168a8]<
      Text(0)"document.url"@12b88a1c8 parent=12b848298 {286,143,4716,750} vis-overflow=-30,0,4776,750 scr-overflow=0,0,4716,750 [state=02010048a0000000] [content=12b344c00] [sc=12fe85ce8:-moz-text] [run=12fc5c2e0][0,12,T] 
    >
    Text(8)" refcounted, see "@12b88a250 parent=12b82a7d8 next=12b848328 {31303,3971,6304,1020} vis-overflow=-30,0,6601,1020 scr-overflow=0,0,6304,1020 [state=02010048a0400000] [content=12b344c80] [sc=12fe0f788:-moz-text] [run=109145000][0,17,T] 
    Inline(a)(9)@12b848328 parent=12b82a7d8 next=12a977020 next-in-flow=12a977020 {37607,3971,0,1020} vis-overflow=-960,-2010,16112,2310 scr-overflow=-960,-2010,16112,2310 [state=0002102000000220] [content=12b33bf50] [sc=129b039a8]<
      Placeholder(_moz_generated_content_before)(-1)@1a01c5918 parent=12b848328 {0,840,0,0} [state=0000000000200040] [content=10b64d940] [sc=130ad6f08:-moz-oof-placeholder] outOfFlowFrame=Block(_moz_generated_content_before)(-1)@12b965260
    >
    AbsoluteList 10a22cec0 <
      Block(_moz_generated_content_before)(-1)@12b965260 parent=12b848328 next=12b961750 {-360,-420,720,720} [state=000010a000d00340] [content=10b64d940] [sc=129b2dac8:before]<
        line 12a8f7e00: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x300] {360,360,0,0} <
          Text(0)""@12b88a2d8 parent=12b965260 {360,360,0,0} [state=4000000028400040] [content=1244d0580] [sc=129b2e018:-moz-text] [run=10b6f7c10][0,0,T] 
        >
      >
      Block(_moz_generated_content_after)(-1)@12b961750 parent=12b848328 {-960,-2010,16112,1650} [state=0002102000d00340] [content=10b64d9d0] [sc=129bbdbc8:after]<
        line 128093b10: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {495,330,15122,990} vis-overflow=465,330,15182,990 scr-overflow=495,330,15122,990 <
          Text(0)"#9782, Use reference-counted URLs more often"@1a01c5a80 parent=12b961750 {495,420,15122,810} vis-overflow=-30,0,15182,810 scr-overflow=0,0,15122,810 [state=00010000a0600040] [content=1247edac0] [sc=129bbdde8:-moz-text] [run=124771820][0,44,T] 
        >
      >
    >
  >
  line 128093ac0: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x0] {0,0,0,0} <
    Inline(a)(9)@12a977020 parent=12b82a7d8 next=12b88a440 prev-in-flow=12b848328 {0,0,0,0} [state=0002100000000607] [content=12b33bf50] [sc=129b039a8]<
      Text(0)"#9782"@1a01c5988 parent=12a977020 next=1a01c5a10 {0,5182,2513,1020} [state=00010000a0200000] [content=12b344d80] [sc=129b02bd8:-moz-text] [run=109145000][0,5,T]  SELECTED
      Placeholder(_moz_generated_content_after)(-1)@1a01c5a10 parent=12a977020 {0,0,0,0} [state=0000000000200442] [content=10b64d9d0] [sc=130ad6f08:-moz-oof-placeholder] outOfFlowFrame=Block(_moz_generated_content_after)(-1)@12b961750
    >
  >
  line 12a8faa20: count=3 state=inline,dirty,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x121] {0,5182,39303,1260} vis-overflow=-30,5182,39600,1260 scr-overflow=0,5182,39303,1260 <
    Text(10)" and "@12b88a440 parent=12b82a7d8 next=12b848df8 {2513,5302,1915,1020} vis-overflow=-30,0,1975,1020 scr-overflow=0,0,1915,1020 [state=02000048a0000000] [content=12b344e00] [sc=12fe0f788:-moz-text] [run=109145000][0,5,T] 
    Inline(a)(11)@12b848df8 parent=12b82a7d8 next=12b88a740 {4428,5302,2928,1020} vis-overflow=-30,0,2988,1020 scr-overflow=0,0,2928,1020 [state=0200100800000200] [content=12b346030] [sc=130e17458]<
      Text(0)"#13790"@12b88a628 parent=12b848df8 {0,0,2928,1020} vis-overflow=-30,0,2988,1020 [state=02000048a0000000] [content=12b344f00] [sc=12fe86bc8:-moz-text] [run=109145000][0,6,T] 
    >
    Text(12)" . Would you like to give some comments on relatio"@12b88a740 parent=12b82a7d8 next=12b9fc3a0 next-in-flow=12b9fc3a0 {7356,5302,31947,1020} vis-overflow=-30,0,32244,1020 scr-overflow=0,0,31947,1020 [state=02000048a1400010] [content=12b344f80] [sc=12fe0f788:-moz-text] [run=109145000][0,82,F] 
  >
  line 12a8fba78: count=1 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x120] {0,6442,39918,1260} vis-overflow=-30,6442,40215,1260 scr-overflow=0,6442,39918,1260 <
    Text(12)"and if we are going to use refcounting eventually,"@12b9fc3a0 parent=12b82a7d8 next=12b9fc5b0 prev-in-flow=12b88a740 next-in-flow=12b9fc5b0 {0,6562,39918,1020} vis-overflow=-30,0,40215,1020 scr-overflow=0,0,39918,1020 [state=0200004891600014] [content=12b344f80] [sc=12fe0f788:-moz-text] [run=109145000][82,103,F] 
  >
  line 12a8fbac8: count=1 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x101] {0,7702,16552,1260} vis-overflow=-30,7702,16612,1260 scr-overflow=0,7702,16552,1260 <
    Text(12)"fragment is possible to change frequently?"@12b9fc5b0 parent=12b82a7d8 prev-in-flow=12b9fc3a0 {0,7822,16552,1020} vis-overflow=-30,0,16612,1020 scr-overflow=0,0,16552,1020 [state=0200004890600014] [content=12b344f80] [sc=12fe0f788:-moz-text] [run=109145000][185,42,T] 
  >
>

There are a couple of things here that concern me: first, notice that although the Placeholder(_moz_generated_content_after) occurs in the expected place, after the text "#9782", the corresponding out-of-flow Block(_moz_generated_content_after) is in the AbsoluteList of the previous line (along with the block from ::before). That seems questionable, and I guess it results in the warning mentioned above, because the out-of-flow gets reflowed when we process the previous line's AbsoluteList, but at that point we haven't yet reflowed its placeholder; and it probably also leads to the later assertion when these frames are being removed.

So it seems like when we break the line (before the ::after placeholder), we ought to be moving the corresponding out-of-flow from the previous line's AbsoluteList and storing it in the new line's list instead, so that it will be reflowed in the proper order.

Doing that would not entirely fix this issue, however, because we'd then have the ::before content (which Github uses to produce the arrow pointing at the link) on the previous line, and the main issue title on the new line. I suspect that would result in broken rendering where the arrow would be detached from the main body of the tooltip.

What I think really needs to happen is that when we decide to break the line before the text of the inline <a> element, we should also ensure the line-break is before any preceding placeholder for an out-of-flow. Then both the ::before and ::after placeholders and their corresponding OOF blocks should end up after the line-break, not before it (in the case of the empty ::before OOF) or separated across it.
Looks like we should either do something while reflowing/placing (a) the ::before pseudo or (b) the inline <a> element.

(a) push the ::before OOF to next line if the inline <a> element can't fit in the same line, maybe do this somewhere near CanPlaceFrame() [1]. Not sure if we can get enough information while placing the ::before pseudo...

or

(b) remove the ::before OOF pseudo from the previous line and add it to the current line if the inline <a> element can't be in the same line with its ::before OOF pseudo. We might need an extra pass to reflow the previous line again....


Also, deciding where to place a "position: absolute;" element that don't specify the top/left coordinates is a bit unclear to me.... I shall probably double check the related specification first. Keep digging to see if I can find a fix....




[1] https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/layout/generic/nsLineLayout.cpp#1085
Sorry, I can't continue to work on this bug. I don't have a working patch for approaches in comment 14 yet, but I think the analysis in comment 13 is very helpful. Anyone who'd like to make progress, please read Comment 13 before going forward.
Assignee: chenpighead → nobody
(In reply to Flore Allemandou [:flore] from comment #10)
> I tried to do a mozregression tonight on this bug to showcase mozregression
> during a nightly workshop. We went back as far as 2012 and it did not work.
> In 2010, there was a visual glitch (blink) and before there was no tooltip.
> 
> We concluded that it never actually worked but nobody reported it (or
> noticed) it before today.

Given this, let's not track as a regression.

Maire, is this still a P2?
Flags: needinfo?(mreavy)
Keywords: regression
Flags: needinfo?(mreavy)
Priority: P2 → P3
Whiteboard: [webcompat]

Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.

Webcompat Priority: --- → ?

See bug 1547409. Migrating whiteboard priority tags to program flags.

Attached file Updated testcase

Here's a further testcase, showing examples where Firefox inappropriately allows a line-break after the placeholder of an abs-pos element adjacent to the start of a word. The issue can be demonstrated both using ::before to insert the abs-pos (as in the original Github example) or using an element that is inline in the text, but styled with position:absolute. Note how in some cases not only the abs-pos element is misplaced, but also the left border of the target span ("jumps") appears at the end of the preceding line.

In Chrome and Safari, the red "before" border and the abs-pos element consistently stay at the left edge of the target span, as would be expected.

(Note that Github appears to have made major changes to their styling of the pages where this was originally reported, so the issue no longer occurs there. However, the underlying Firefox bug is still present, as demonstrated in the attached testcases, and may still be affecting other sites.)

Summary: Wrong position of github issue name on hover → abs-pos placeholder may end up on the wrong side of a line-break [was: Wrong position of github issue name on hover]

The problem here arises because by the time we discover that the "target" word (e.g. the issue number in the original Github example) doesn't fit on the line, we've already appended the preceding placeholder frame to the nsLineLayout that we're building. And if it was generated by the ::before pseudo of a span, or if it occurred immediately after the beginning of a real <span> element, we'll also have begun that span on the line before the break, even though none of its actual content ends up fitting.

There's a fairly simple fix for the worst of this, it seems: if we force the BuildTextRunsScanner to do a FlushFrames() when it encounters the placeholder, then the potential line-break before the placeholder will be correctly recorded. The downside of this is that it will interrupt the textrun at this point, which means that shaping (effects like ligatures and kerning) will break across the position of the abs-pos placeholder, even if the possible line-break ends up not being used.

This also affects the implementation of text-transform:capitalize, meaning that a placeholder within a word would cause the following letter to be treated as the start of a new word, and hence capitalized. However, we can work around that by recording whether the line-breaker is in the middle of a word at the time of calling FlushFrames, and if so, suppress capitalization of the first letter after the flush.

A better fix might be possible if we could loosen the coupling between scanning frames in BuildTextRunsScanner and appending them to the nsLineLayout, so that we don't actually begin a new span within the line, or add the placeholder to it, until we've determined that we are actually adding in-flow content as well. But this seems much more invasive and risky, so I'm inclined to start with the simple fix. We can also add a test to cover this example, so that we can check for any changes in behavior if/when we restructure the code more extensively.

Attachment #9066458 - Attachment is patch: false
Assignee: emilio → jfkthame
Attachment #9066458 - Attachment mime type: text/plain → text/html

Bug 1553418 discusses the issue raised by the attachment 9066458 [details] testcase, which is a somewhat separate (pre-existing) bug/interop issue.

See Also: → 1553418
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/03ffbd776c1e
Add WPT reftests for the abs-pos at line-break issue here. r=emilio
https://hg.mozilla.org/integration/autoland/rev/97a9fa27755b
Flush line-breaks when encountering a Placeholder for an absolutely-positioned frame. r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16948 for changes under testing/web-platform/tests
Whiteboard: [webcompat] → [webcompat][wptsync upstream]
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Regressions: 1837272
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: