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)

Product:
Core
Component:
CSS Parsing and Computation
Platform:
Unspecified
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: chenpighead, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

Bug 1369321. Make sure to restyle from all our style roots when rebuilding all style data with stylo.
59 bytes, text/x-review-board-request
bholley
: review+
Details 
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.
Hmmm.... scrollbars of <textarea> looks fine...

data:text/html, <textarea name="textarea" rows="2" cols="50">Write something here</textarea>
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...
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">
(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!!!
Priority: -- → P2
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)
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
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)
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...
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.
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.
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.
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.
(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
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.
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).
Assignee: jeremychen → bzbarsky
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+
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.
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
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: