Closed
Bug 1369321
Opened 7 years ago
Closed 7 years ago
stylo: Viewport scrollbar should not be zoomed while zooming the whole content document
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: chenpighead, Assigned: bzbarsky)
References
Details
Attachments
(1 file)
STR: 1. open following test with stylo build ``` data:text/html,<div style="width: 200%; height: 20px; background: blue"></div> ``` 2. press 'ctrl' + '+' to do the global zoom-in 3. repeat step 2 until we reach the maximum (seems like 300% is the max) Expect: The size of horizontal scrollbar is consistent. Actual: The size of horizontal scrollbar is zoomed as well. I found this while trying re-enable the layout/reftests/image/image-srcset-svg-3x.html test. This is more like a scrollbar issue than a SVG one, so I'm not setting this as a blocker for stylo-svg. Here's a try result to see the difference between stylo and gecko: https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/TV_V6NeaRqSvzBH4ypIzYg/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1 The interesting part is, this is reproducible on Linux, but not on Mac.
Reporter | ||
Comment 1•7 years ago
|
||
Hmmm.... scrollbars of <textarea> looks fine... data:text/html, <textarea name="textarea" rows="2" cols="50">Write something here</textarea>
Reporter | ||
Comment 2•7 years ago
|
||
scrollbars of <div> works fine as well... data:text/html,<div style="overflow: scroll; width: 200%; height: 20px; background: blue"></div> Perhaps it's a re-style issue just for root document...
Comment 3•7 years ago
|
||
Can you compare SVG as a top level document with scroll bars with HTML as a top level document with scroll bars? <svg xmlns="http://www.w3.org/2000/svg" width="3000" height="3000"></svg> vs <html style="overflow: scroll">
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #3) > Can you compare SVG as a top level document with scroll bars with HTML as a > top level document with scroll bars? > > <svg xmlns="http://www.w3.org/2000/svg" width="3000" height="3000"></svg> > > vs > > <html style="overflow: scroll"> Both of them have this issue!!!
Updated•7 years ago
|
Priority: -- → P2
Reporter | ||
Comment 5•7 years ago
|
||
Here's the frame tree dumped from comment 0: ==== Viewport(-1)@1275a70c8 [view=1243a8600] {0,0,36600,22920} [state=0000062000002220] [sc=1275a7158:-moz-viewport]< HTMLScroll(html)(-1)@1275a7360 {0,0,36600,22920} [state=0280020800000010] [content=10c1e5030] [sc=1275a7018:-moz-viewport-scroll]< ScrollbarFrame(scrollbar)(-1)@1275a76d0 next=1275a7d60 {0,21960,36600,960} [state=0000104080c80008] [content=10bf4bf20] [sc=12815e478]< SliderFrame(slider)(-1)@1275a7b08 {0,0,36600,960} [state=0000164080c00000] [content=1240661a0] [sc=12815e528]< ButtonBoxFrame(thumb)(0)@1275a7c98 {60,0,16590,960} [state=2000164080400000] [content=124067670] [sc=12815e7d8]<> > > ScrollbarFrame(scrollbar)(-1)@1275a7d60 next=1275a83f0 {36600,0,0,22920} [state=0000100080880008] [content=10dd75940] [sc=12815e5d8]< SliderFrame(slider)(-1)@1275a8198 {0,0,960,22920} [state=0000160080800000] [content=1241f9a60] [sc=12815e688]< ButtonBoxFrame(thumb)(0)@1275a8328 {0,60,960,22800} [state=2000160080000000] [content=1241f9f70] [sc=1275a8d98]<> > > Box(scrollcorner)(-1)@1275a83f0 next=1275a72b8 {36600,22920,0,0} [state=0000100080c00208] [content=1240238f0] [sc=1275a7620]<> Canvas(html)(-1)@1275a72b8 {0,0,36600,22920} vis-overflow=0,0,71760,22920 scr-overflow=0,0,71760,22920 [state=0000006800000210] [content=10c1e5030] [sc=1275a84a8:-moz-scrolled-canvas]< Block(html)(-1)@1275a8608 {0,0,36600,2160} vis-overflow=0,0,71760,2160 scr-overflow=0,0,71760,2160 [state=0200100800d00210] [content=10c1e5030] [sc=1275a8558]< line 1275a8a28: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x48] bm=480 {480,480,35640,1200} vis-overflow=480,480,71280,1200 scr-overflow=480,480,71280,1200 < Block(body)(1)@1275a8978 {480,480,35640,1200} vis-overflow=0,0,71280,1200 scr-overflow=0,0,71280,1200 [state=0200120800100200] [content=10bf4be90] [sc=1275a8768]< line 1275a8b28: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x8] {0,0,71280,1200} < Block(div)(0)@1275a8a78 {0,0,71280,1200} [state=020012c800100210] [content=12556e3a0] [sc=1275a88c8]< > > > > > > > > ==== I noticed that the viewport scrollbar is using a -moz-viewport-scroll style context. Also, from https://public.etherpad-mozilla.org/p/anon-box-stylo, looks like we haven't support ::-moz-viewport-scroll yet. Hi Boris, does this mean we need to support incremental restyle for ::-moz-viewport-scroll anonymous boxes to resolve this issue? Do you think this is related to Bug 1292609?
Flags: needinfo?(bzbarsky)
![]() |
Assignee | |
Comment 6•7 years ago
|
||
That's entirely possible, yes. I hadn't managed to find testcases that broke without dynamic restyling support on the root frames, so I had not prioritized it. I can just fix them and see whether it helps this problem.
Blocks: 1292609
![]() |
Assignee | |
Comment 7•7 years ago
|
||
OK. The patches in bug 1374761 restyle the root scrollframe itself but do not seem to help this bug. This is not terribly surprising now that I've read this code more carefually, because restyling a scrollframe will restyle itself and its owned anon box (the scrollframe) but not do anything with the scrollbars themselves. Those are handled via AllChildrenIterator normally. And AllChildrenIterator::AppendNativeAnonymousChildren has a special case to handle the viewport scrollbars: it calls nsContentUtils::AppendDocumentLevelNativeAnonymousContentTo which explicitly appends the root scrollbars to the list so they look like kids of the root element and should get restyled accordingly.
Flags: needinfo?(bzbarsky)
![]() |
Assignee | |
Comment 8•7 years ago
|
||
Oddly enough, the patches in bug 1374761 do fix a few reftests that use reftest-zoom and were failing because Gecko didn't zoom the viewport scrollbars but stylo did. Specifically, the following tests end up fixed: layout/reftests/image/image-srcset-basic-selection-width-10x.html layout/reftests/image/image-srcset-svg-3x.html layout/reftests/transform-3d/animate-cube-radians-zoom.html layout/reftests/transform-3d/animate-cube-degrees-zoom.html but the steps to reproduce for this bug still show the problem for me...
![]() |
Assignee | |
Comment 9•7 years ago
|
||
So I tried to experiment a bit. If I change zoom levels in a non-e10s window by doing: getBrowser().docShell.contentViewer.fullZoom = 10 in a chrome scratchpad (which is what the reftest harness effectively does), then the scrollbars do NOT get zoomed with the document itself. But if I change them via the menu or keyboard shortcut, then they do. And if I use the keyboard shortcut to get to a fullZoom of 2, then run the above line, the scrollbars get to be 10x the size. I dug into what the keyboard shortcut does a bit, and it just ends up calling into http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/toolkit/content/widgets/browser.xml#538-545 which should be the same as the above. I tried also resetting the text zoom, as it does, but the scrollbars still don't zoom. OK, so I tried doing: ZoomManager.setZoomForBrowser(getBrowser(), 3); in the scratchpad, and the scrollbars still don't get zoomed. And if I do: ZoomManager.zoom = 3; they also don't get zoomed. However if I use: ZoomManager.enlarge(); they do get zoomed. And if I just walk through the list of zooms (1.1, 1.2, 1.33, 1.5, 1.7, 2, 2.4, 3) and do ZoomManager.zoom = val for each one, the scrollbars get zoomed. But not if I go directly from 1 to 3.
![]() |
Assignee | |
Comment 10•7 years ago
|
||
To be clear, comment 9 is all with the patches from bug 1374761 applied. Without those patches, the scrollbars get zoomed even if I do a direct: getBrowser().docShell.contentViewer.fullZoom = 10; which means those patches are definitely necessary to get the right behavior, but are not sufficient for some reason.
![]() |
Assignee | |
Comment 11•7 years ago
|
||
Oh, and if I do the walk through the list like so: for (var zoom of [1.1, 1.2, 1.33, 1.5, 1.7, 2, 2.4, 3]) getBrowser().docShell.contentViewer.fullZoom = zoom; then the zoom ends up being 3 and the scrollbar is not zoomed. But if I do: var gen = (function* () { yield* [1.1, 1.2, 1.33, 1.5, 1.7, 2, 2.4, 3] })() function f() { var val = gen.next(); if (!val.done) { getBrowser().docShell.contentViewer.fullZoom = val.value; setTimeout(f, 100) } } f(); then I see the scrollbars zooming.
![]() |
Assignee | |
Comment 12•7 years ago
|
||
OK, so what we do on zoom change is that nsPresContext::AppUnitsPerDevPixelChanged calls: MediaFeatureValuesChanged(eRestyle_ForceDescendants, NS_STYLE_HINT_REFLOW); If I change that to pass eRestyle_Subtree then the scrollbars stop zooming. So it sounds like Gecko considers the root scrollbars "descendants" of the root for purposes of the restyle hints and stylo fails to do so.
Comment 13•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #12) > If I change that to pass eRestyle_Subtree then the scrollbars stop zooming. > So it sounds like Gecko considers the root scrollbars "descendants" of the > root for purposes of the restyle hints and stylo fails to do so. Yeah, we treat the root scrollbars as separate "style roots": http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/layout/style/ServoStyleSet.cpp#924
![]() |
Assignee | |
Comment 14•7 years ago
|
||
Oh, and StyleChildrenIterator uses eSkipDocumentLevelNativeAnonymousContent so skips them. That's the part I was missing. Sounds like the real bug is then that ServoRestyleManager::PostRebuildAllStyleDataEvent doesn't post hints for all the style roots. Let me test that hypothesis.
![]() |
Assignee | |
Comment 15•7 years ago
|
||
That said, I now no longer understand why the patches from bug 1374761 change behavior here, unless they're causing more reframing of the root or something (which would of course be bad too).
Comment hidden (mozreview-request) |
![]() |
Assignee | |
Updated•7 years ago
|
Assignee: jeremychen → bzbarsky
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8880087 [details] Bug 1369321. Make sure to restyle from all our style roots when rebuilding all style data with stylo. https://reviewboard.mozilla.org/r/151440/#review156382
Attachment #8880087 -
Flags: review?(bobbyholley) → review+
![]() |
Assignee | |
Comment 18•7 years ago
|
||
Continuing with the confusion... with the patches from bug 1374761 (but without the patch attached here), if I log what elements make it into compute_style in traversal.rs, I don't see the scrollbar bits when changing the zoom level to 10. And I don't see obvious frame reconstructs either. But the scrollbar also doesn't get bigger. I don't understand why.
![]() |
Assignee | |
Comment 19•7 years ago
|
||
Ah, I see, sort of. The very first time zoom is set, without this patch but with the patch for bug 1374761, we somehow think this is the "right" zoom level now. And it shows the scrollbar at the normal size, but smaller zooms show it smaller (e.g. going from 10 to 3 shows a very narrow scrollbar) and larger ones show it larger. That happens with the 1.1, etc sequence too, but 1.1 is close enough to 1 that it all looks like we're just increasing all the time. I'm still not sure why we get that "right level" confusion, but given that we're not reframing I'll stop spending time on sorting it out. The patch in this bug fixes one additional test: layout/reftests/border-dotted/border-dashed-radius-zoom.html
Comment hidden (mozreview-request) |
Comment 21•7 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4729549d9e8b Make sure to restyle from all our style roots when rebuilding all style data with stylo. r=bholley
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4729549d9e8b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•