Closed
Bug 1343298
Opened 7 years ago
Closed 7 years ago
Remove nsHTMLScrollFrame::ReloadChildFrames() because it's redundant.
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
Details
Attachments
(1 file)
3.10 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
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.)
Assignee | ||
Comment 2•7 years ago
|
||
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...
Comment 3•7 years ago
|
||
(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...
Assignee | ||
Comment 4•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Component: XUL → Layout
Summary: Implement nsXULScrollFrame::ReloadChildFrames() if needed (compare with nsHTMLScrollFrame) → Remove nsHTMLScrollFrame::ReloadChildFrames() because it's redundant.
Assignee | ||
Comment 5•7 years ago
|
||
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)
Assignee | ||
Comment 6•7 years ago
|
||
(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.)
Comment 7•7 years ago
|
||
(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 8•7 years ago
|
||
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
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/beca679d682b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•