Closed Bug 1343298 Opened 3 years ago Closed 3 years ago

Remove nsHTMLScrollFrame::ReloadChildFrames() because it's redundant.

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: mats, Assigned: mats)

Details

Attachments

(1 file)

Follow-up from bug 1342801 comment 3:
> ::: layout/generic/nsGfxScrollFrame.h
> @@ +1073,5 @@
> > +    mHelper.ReloadChildFrames();
> > +    if (mHelper.mScrolledFrame) {
> > +      mWritingMode = mHelper.mScrolledFrame->GetWritingMode();
> > +    }
> > +  }
> 
> Don't we need to use a similar wrapper for mHelper.ReloadChildFrames in
> nsXULScrollFrame as well?
> 
> Hmm, but we don't currently have a GetWritingMode override there, so maybe
> it's not needed. Or maybe that's a bug we should fix. Although we don't
> support vertical writing modes in XUL, we should support bidi, and the bidi
> direction value becomes part of what GetWritingMode returns. So unless
> there's a reason why the nsXULScrollFrame doesn't need this behavior, it
> kinda looks like we might have a bug that we just haven't noticed until now.
nsHTMLScrollFrame used to override GetWritingMode() and return the scrolled frame's value.
However, nsXULScrollFrame did not have that override for some reason, but as Jonathan
notes, that looks like an oversight.  (Now that bug 1342801 is implemented though,
we need something like nsHTMLScrollFrame::ReloadChildFrames() on nsXULScrollFrame.)
It appears that both nsHTMLScrollFrame and nsXULScrollFrame is the primary
frame for its element, so if the UA style rules are correct then we should
inherit all the writing-mode/direction stuff to the anonymous frames, right?
So I'm wondering why nsHTMLScrollFrame::GetWritingMode() was needed in
the first place...

Fwiw, I tested XUL <textbox id="t" style="direction:rtl"> ... and
<hbox style="direction:rtl; overflow:scroll;"> and the frame dumps
show "wm=h-rtl" on all the relevant frames...
(In reply to Mats Palmgren (:mats) from comment #2)
> It appears that both nsHTMLScrollFrame and nsXULScrollFrame is the primary
> frame for its element, so if the UA style rules are correct then we should
> inherit all the writing-mode/direction stuff to the anonymous frames, right?
> So I'm wondering why nsHTMLScrollFrame::GetWritingMode() was needed in
> the first place...

Hmm, at this point I'm not sure, but clearly at the time it seemed important. It looks like that comes from bug 1108067, so I guess one experiment would be to remove that and then try the testcases from that bug and see if anything shows up...
The testcases in that bug seems to work fine without it, so I pushed it to Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c2fedfa87309d12245487c5e5ca057f7482c768
Component: XUL → Layout
Summary: Implement nsXULScrollFrame::ReloadChildFrames() if needed (compare with nsHTMLScrollFrame) → Remove nsHTMLScrollFrame::ReloadChildFrames() because it's redundant.
The scroll frame is almost always the content's primary frame and if so
it already has the correct style values and the nsFrame ctor has set
mWritingMode correctly based on those.  For the edge cases where it's
not the primary frame, e.g. <fieldset style=overflow:scroll>, the UA
sheet specifies 'inherit' for the relevant properties so it has
the correct style values in this case too.
Assignee: nobody → mats
Attachment #8842358 - Flags: review?(jfkthame)
(The Try run in comment 4 have some failures but I think those are from
the m-i revision I based it on.  I'm pretty sure it's not from this change.)
(In reply to Mats Palmgren (:mats) from comment #2)
> It appears that both nsHTMLScrollFrame and nsXULScrollFrame is the primary
> frame for its element, so if the UA style rules are correct then we should
> inherit all the writing-mode/direction stuff to the anonymous frames, right?
> So I'm wondering why nsHTMLScrollFrame::GetWritingMode() was needed in
> the first place...

I was curious enough about this that I pushed a try job based on the changeset immediately -before- bug 1342801, with nsHTMLScrollFrame::GetWritingMode() removed:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ee4b4f57cfe8d1a6a4e8494dc7a92289b428f3f

That provides plenty of failures, showing that the GetWritingMode() override was definitely playing an important role at that point. However, those failures don't reappear on your try job here, suggesting that the new approach is perhaps doing a more thorough job of propagating writing-mode than the old per-frame-class overrides.
Comment on attachment 8842358 [details] [diff] [review]
Remove nsHTMLScrollFrame::ReloadChildFrames() because it's redundant.

Review of attachment 8842358 [details] [diff] [review]:
-----------------------------------------------------------------

I agree, the try failures don't look related to this at all.
Attachment #8842358 - Flags: review?(jfkthame) → review+
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/beca679d682b
Remove nsHTMLScrollFrame::ReloadChildFrames() because it's redundant.  r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/beca679d682b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.