1024454 - No reflow triggered after font load, causing icon font glyph to appear offset from correct position.

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect)

Description

[xfinx]

Attachment: FX - LC - arrow - wrong.png

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140605174243

Steps to reproduce:

I went to visit https://en.leadingcourses.com/. 


Actual results:

The initial load show an icon font glyph on the wrong location. After a normal refresh (hitting refresh button) the icon font glyph is positioned correctly. If I change font size or something in firebug, the icon font glyph (arrow) is positioned correctly.


Expected results:

icon font glyph should be centered in wrapping anchor.

Comment 1

[Sander]

This regressed between 20140311030201 - https://hg.mozilla.org/mozilla-central/rev/41d962d23e81 and 20140312030201 - https://hg.mozilla.org/mozilla-central/rev/44ae8462d6ab - http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=41d962d23e81&tochange=44ae8462d6ab

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

(In reply to Sander from comment #1)
... and further

No more inbounds to bisect
Last good revision: a2f674b6536c
First bad revision: 63c25c5fa758
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a2f674b6536c&tochange=63c25c5fa758

Comment 3

[Sander]

Attachment: Minimal testcase

Comment 4

[(mostly gone) XtC4UaLL [:xtc4uall]]

(In reply to XtC4UaLL [:xtc4uall] from comment #2)
disregard that range. seem like the recent added inbound archive check in mozregression is unreliable ...

Comment 5

[Mats Palmgren (inactive)]

The regression range in comment 1 includes bug 63895, and the testcase has
a table cell as the absolute containing block.  Do we have -inbound builds
before / after that bug to confirm?

Comment 6

[(mostly gone) XtC4UaLL [:xtc4uall]]

On mozilla-inbound using builds taken off http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/

1394489627 f4911ad70a26 -> testcase ok,    site ok
1394491309 84e4b2ecb19b -> testcase blank, site broken

=> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f4911ad70a26&tochange=84e4b2ecb19b
=> Bug 63895 (Part 1-3)

On mozilla-central the testcase got blank -> non-blank/current state within
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3bc3b9e2cd99&tochange=e84a391b604b
=> Bug 63895 (Part 4-5)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Seth, could you take a look?

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Regression, tracking. However, we won't make a dot release for 30 for this.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Untracking for 31. We won't block the release for this bug.
Seth, it would be nice if you could have a look for the 32 release.

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Jet - You told me that you wanted to fix this bug in beta. I don't see any progress in the bug. Is there someone who can take a look tomorrow to see about the feasibility of putting a fix together on Wed?

Comment 11

[Seth Fowler [:seth] [:s2h]]

I'm literally working on this bug right now. Unfortunately a fix is not straightforward, but I'll continue to push on it.

Comment 12

[Seth Fowler [:seth] [:s2h]]

I've looked into this, and I have a patch that works, but has performance implications. Tomorrow I'm going to see what I can do about making it more efficient.

The problem is not actually that no reflow is triggered. The same sequence of reflows is triggered regardless of whether the table part acts as an absolute containing block. What appears to be happening is that we are clearing the dirty bits during the normal table reflow, so when we get to FixupPositionedTableParts, we don't actually reflow div that contains the icon glyph.

There are some details that are still unclear to me, but I can solve the problem currently just marking the relatively-positioned frame as dirty before reflowing its absolutely-positioned children in FixupPositionedTableParts. Doing that unconditionally is not obviously preferable, though, and I suspect that we should be able to solve this problem by leaving the appropriate dirty bits in place when we do the first reflow pass, which would be more precise than my current solution.

I'll take a look again tomorrow and see if I can't put together a better patch than my current solution. What I have *is* shippable, though, if it turns out it's not easy to do better.

Comment 13

[Seth Fowler [:seth] [:s2h]]

OK, I have a more precise fix. (As I mentioned above, it boils down to leaving the appropriate dirty bits in place during the initial reflow.) It requires a rebase against bug 35168, which I'm currently preparing to push, but I should have a patch to upload shortly.

Comment 14

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Eagerly propagate dirty bits so absolute children of table parts get reflowed reliably

Alright, this patch does the job. It propagates dirty bits to absolutely-positioned children of relatively-positioned table parts so that when we come around in a later pass to reflow them, they're correctly marked as dirty. This is necessary because nsHTMLReflowState would ordinarily propagate the dirty bits from the parent, but the parent's dirty bit gets cleared by earlier reflow passes before we get to the point where we call FixupPositionedTableParts.

Comment 15

[Seth Fowler [:seth] [:s2h]]

I'd like to check in a test for this as well, though of course we shouldn't delay part 1 to do so. jfkthame had the following advice when I asked when which in-tree fonts would work well for this test:

> jfkthame: how about using a couple of characters such as "i" and "m" from a proportional
> font - Gentium or LinLibertine or DejaVu Sans, for example - and setting the fallback to 'monospace'
> jfkthame: that pretty much guarantees something'll change when the font gets loaded, i think
> jfkthame: btw, for a reftest you might need to deliberately delay the font load, e.g by adding the
> style rule dynamically after the test initially loads

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8475633 [details] [diff] [review]
(Part 1) - Eagerly propagate dirty bits so absolute children of table parts get reflowed reliably

>+void
>+nsFrame::PushDirtyBitToAbsoluteFrames()
>+{
>+  if (!(GetStateBits() & NS_FRAME_IS_DIRTY)) {
>+    // No dirty bit to push.
>+    return;
>+  }
>+  if (HasAbsolutelyPositionedChildren()) {
>+    nsAbsoluteContainingBlock* absoluteContainer = GetAbsoluteContainingBlock();
>+    absoluteContainer->MarkAllFramesDirty();
>+  }
>+}

No need for the |absoluteContainer| local variable; you can just write:

  GetAbsoluteContainingBlock()->MarkAllFramesDirty();

inside the if, of course.


It also seems a little odd to have one condition be an early return and the other be an if around the MarkAllFramesDirty() call -- maybe pick one or the other for both of them?  (Definitely best to check the NS_FRAME_IS_DIRTY bit before the HasAbsolutelyPositionedChildren() call as you do now, though, since it's by far the faster check.)


r=dbaron

Comment 17

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Eagerly propagate dirty bits so absolute children of table parts get reflowed reliably

Thanks David. I've updated the patch.

Comment 18

[Seth Fowler [:seth] [:s2h]]

I wanted to land this after bug 35168, but we're running out of time, so I've gone ahead and pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea0bc2228058

Comment 19

[Seth Fowler [:seth] [:s2h]]

Marking leave-open until the test lands.

Comment 20

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8476125 [details] [diff] [review]
(Part 1) - Eagerly propagate dirty bits so absolute children of table parts get reflowed reliably

Approval Request Comment
[Feature/regressing bug #]: 63895
[User impact if declined]: Incorrect layout when dynamic changes are combined with relatively positioned table parts.
[Describe test coverage new/current, TBPL]: Test will follow but isn't ready yet. Given the limited time available for this to make beta, it's not worth waiting for the test. This patch looks good on mozilla-inbound but hasn't hit mozilla-central; I would ordinarily wait for it to land on m-c but again given the time limitations I think we should just uplift.
[Risks and why]: This is a low-risk patch. All it does is make us mark some frames dirty that we weren't marking as dirty before. This is unlikely to cause a regression since if dirty bit changes were to cause a layout issue it would almost certainly be because they make things dirty *less* aggressively, not because they make them dirty *more* aggressively, as in this patch.
[String/UUID change made/needed]: None.

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ea0bc2228058

Comment 22

[Seth Fowler [:seth] [:s2h]]

Alright, the patch has now landed on m-c and I've verified the fix there.

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8476125 [details] [diff] [review]
(Part 1) - Eagerly propagate dirty bits so absolute children of table parts get reflowed reliably

Thank you for verifying Seth. Approved for aurora and beta.

Comment 24

[Seth Fowler [:seth] [:s2h]]

Thanks, Lawrence! Marking this checkin-needed because I'm going to be AFK for a while. If nobody has done it yet I'll do the uplift in a couple of hours.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f3992ef70443
https://hg.mozilla.org/releases/mozilla-beta/rev/8e6b808eed02

Comment 26

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Reproduced the original issue on Win 7 x64 using the minimal test case from comment 3, with Firefox 30.

The issue no longer reproduced on Win 7 x64, Mac OS X 10.8.5 and Ubuntu 13.04 x64, using Firefox 32 RC build 1 (BuildID=20140825202822). No more issue was seen for the minimal testcase or for the original URL.

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:svoisen, maybe it's time to close this bug?