Closed Bug 849407 Opened 7 years ago Closed 7 years ago

<legend> w/ display:flex overflow:scroll sometimes renders without scrollbars

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(3 files)

Attached file testcase 1
STR:
 1. Load attached testcase.
 2. (optional) Reload a few times.

ACTUAL RESULTS: Some fraction of the time (over 50%), the testcase loads without scrollbars around the blue region.

EXPECTED RESULTS: Scrollbars around the blue region.


NOTE: "display:flex" is required to trigger this, but no flex container actually ends up being generated -- the <legend> produces a nsRenderingFrame.  The problems here stem from the style-changes (purely at a style level) that "display:flex" imposes on its children, which in this case include the scrollbars.

Basically, what happens here is that in our initial load, we know that we need to ignore the flex-item fixups on the scrollbars (from bug 812822's fix).

However, sometimes we end up getting a call to nsPresContext::DoChangeCharSet(), after we've done our initial reflow -- and this calls RebuildAllStyleData, which ends up making us call nsFrameManager::ReResolveStyleContext for everything. And when we do that for our scrollbar frames, we incorrectly apply flex-item style fixups and convert these scrollbars to "display:block".

We just need to create another AutoFlexItemStyleFixupSkipper in the right place to catch this code-path, I think.

(Setting this as blocking bug 782441, since we end up hitting a variant of this bug for *actual* flex containers that have overflow set, too, once I've applied that bug's first patch.)
(In reply to Daniel Holbert [:dholbert] from comment #0)
> ACTUAL RESULTS: Some fraction of the time (over 50%), the testcase loads
> without scrollbars around the blue region.

(side-note: the non-determinacy here seems to be due to the timing of the asynchronous call to  nsPresContext::DoChangeCharSet())
Attached patch reftest patchSplinter Review
Here's a reftest -- it's just two copies of the attached testcase, with "display: flex" removed in the reference case.

It doesn't fail very reliably in un-patched builds, due to the non-determinacy mentioned above, but it did fail a few times in a local reftest run (using a handmande manifest that just includes ~20 copies of the same line, for this reftest).
Attachment #723038 - Flags: review?(bzbarsky)
For reference, here's a Try run w/ the reftest on its own (expected to fail intermittently):
  https://tbpl.mozilla.org/?tree=Try&rev=7794ce946859
  (failed in 3 of ~28 builds)

...and here's a Try run w/ the reftest and the fix (expected to pass):
  https://tbpl.mozilla.org/?tree=Try&rev=69b281ebd76f
  (passed in all ~28 builds)

So the reftest is testing what it's supposed to test (albeit with a low sensitivity).
Comment on attachment 722963 [details] [diff] [review]
fix v1

r=me, but do we need similar things in style context reparenting?
Attachment #722963 - Flags: review?(bzbarsky) → review+
Comment on attachment 723038 [details] [diff] [review]
reftest patch

r=me
Attachment #723038 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #5)
> r=me, but do we need similar things in style context reparenting?

I'm not sure. I hacked up a variant of the attached testcase that moves the <fieldset> element from the <body> to another div called "newParent" that has this selector:
    div#newParent > * > * > * {
      background: lime;
    }
This makes it change the background-color of the inner div inside the scrollframe, after the fieldset has been moved, which let me verify that the reparenting actually happened.  However -- this node-reparenting didn't actually trigger any calls to  nsStyleSet::ReparentStyleContext or nsFrameManager::ReResolveStyleContext, even though the background-color did change.  That seems a bit odd.  (Maybe this ended up being a case where we didn't bother to do reparenting, and instead just blew away the existing style contexts and made new ones...?)

Anyway -- stepping back a bit, note that we only need to skip flex-item style fixup for children of "special" frames -- e.g. a scrollframe's scroll widgets, <input>/<button> helper-frames -- and those children are generally tied to their parent. They're not going to be reparented. (depending on our definition of reparenting).  So perhaps that means we don't have to worry about style context reparenting at all for this stuff?

In any case -- I'll land the attached patch, and if it turns out that we need similar code for style-context reparenting, we can track that in a new bug.
Folded reftest patch into the code-fix patch, and landed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/03c1bff64491
Flags: in-testsuite? → in-testsuite+
To trigger style context reparenting you need either things moving to/from ::first-line or a style change on an element (which then causes all its descendants' style contexts to be reparented).
https://hg.mozilla.org/mozilla-central/rev/03c1bff64491
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 863530
You need to log in before you can comment on or make changes to this bug.