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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
5 years ago
9 months ago

People

(Reporter: bjorn, Assigned: seth)

Tracking

({regression, testcase})

30 Branch
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox30- wontfix, firefox31+ wontfix, firefox32+ verified, firefox33+ fixed, firefox34 fixed, firefox-esr60 unaffected, firefox65 unaffected)

Details

()

Attachments

(3 attachments, 1 obsolete attachment)

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.
Keywords: regression
Component: Untriaged → Layout
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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
Posted file Minimal testcase
(In reply to XtC4UaLL [:xtc4uall] from comment #2)
disregard that range. seem like the recent added inbound archive check in mozregression is unreliable ...
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?
Keywords: testcase
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)
Blocks: 63895
Component: Layout → Layout: R & A Pos
Seth, could you take a look?
Flags: needinfo?(seth)
Regression, tracking. However, we won't make a dot release for 30 for this.
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.
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?
Flags: needinfo?(bugs)
I'm literally working on this bug right now. Unfortunately a fix is not straightforward, but I'll continue to push on it.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(seth)
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.
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.
Attachment #8475633 - Flags: review?(dbaron)
Assignee: nobody → seth
Status: NEW → ASSIGNED
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 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
Attachment #8475633 - Flags: review?(dbaron) → review+
Attachment #8475633 - Attachment is obsolete: true
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
Marking leave-open until the test lands.
Keywords: leave-open
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.
Attachment #8476125 - Flags: approval-mozilla-beta?
Attachment #8476125 - Flags: approval-mozilla-aurora?
Alright, the patch has now landed on m-c and I've verified the fix there.
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.
Attachment #8476125 - Flags: approval-mozilla-beta?
Attachment #8476125 - Flags: approval-mozilla-beta+
Attachment #8476125 - Flags: approval-mozilla-aurora?
Attachment #8476125 - Flags: approval-mozilla-aurora+
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.
Keywords: checkin-needed
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.
The leave-open keyword is there and there is no activity for 6 months.
:svoisen, maybe it's time to close this bug?
Flags: needinfo?(svoisen)
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Flags: needinfo?(svoisen)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.