Closed Bug 1471708 Opened 6 years ago Closed 6 years ago

Rename the "scroll-position clamping scroll port size" to the "visual viewport size"

Categories

(Core :: Panning and Zooming, enhancement, P3)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: botond, Assigned: o0ignition0o, Mentored)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [gfx-noted] [lang=c++])

Attachments

(1 file)

Bug 1471708 - Rename the "scroll-position clamping scroll port size" to "visual viewport size".
59 bytes, text/x-review-board-request
botond
: review+
Details 
We have a quantity that Layout code works with called the "scroll-position clamping scroll port size".

Conceptually, this represents the amount of web content that's visible in the browser's content area at one time, when you're zoomed in (as contrasted to the size into which the page is reflowed, which can be larger in the presence of zooming).

Now that we're cleaning up / reworking some of the code surrounding viewport-related concepts in bug 1123938 and dependent bugs, in other places we've been calling this quantity the "visual viewport size".

To keep our naming consistent, I would like to rename our current "scroll-position clamping scroll port size" to "visual viewport size".
Depends on: 1470267
I would like to work on this one, can I give it a go ? 

I will probably start hacking on it next Monday if you don't mind
(In reply to Jeremy Lempereur [:o0ignition0o] from comment #1)
> I would like to work on this one, can I give it a go ? 

For sure! I assigned the bug to you.

> I will probably start hacking on it next Monday if you don't mind

Sounds good.

The main place where we store the SPC-SPS is in the pres shell [1], but there are a variety of related methods that we'll want to rename as well.

[1] https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/layout/base/nsIPresShell.h#1746
Assignee: nobody → jeremy.lempereur
Is there a full description of the various different viewports, and their relation to each other (with examples!)?

It would be really nice if there were a wiki page explaining the model that we could link to in code.
The best documentation that I'm aware of is this explainer by David Bokan from the Chromium team:

https://github.com/bokand/bokand.github.io/blob/master/web_viewports_explainer.md

It explains the current differences in behaviour related to viewports between browsers. Our tracking bug for resolving these differences is bug 1123938.
Blocks: viewport-compat
Blocks: 1470267
No longer depends on: 1470267
Hi Jeremy, have you had a chance to take a look at this? Let me know if there's anything I can clarify / help with.
Flags: needinfo?(jeremy.lempereur)
Hi :)

I'm sorry I didn't have time to have a look at it yet, I'm willing to have a look at it this weekend if it's possible.
Else I'll just grab an other one :)
Flags: needinfo?(jeremy.lempereur)
Sure, sounds good!
Ok so I've started renaming occurrences of ScrollPositionClampingScrollPortSize to VisualViewportSize but I'm wondering how I should reason about generated code, especially Rust code : 

> https://searchfox.org/mozilla-central/source/__GENERATED__/dist/xpcrs/rt/nsIDOMWindowUtils.rs#491


and 

> https://searchfox.org/mozilla-central/source/__GENERATED__/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-f022f4a96002686e/out/gecko/structs.rs#33045

The latter seems to be a bindgen so it would probably be auto generated once the patch lands, but I'd rather make sure I'm not doing anything wrong ^^
Flags: needinfo?(botond)
Yes, both of the mentioned uses will be re-generated from the corresponding in-tree source files (in this case, [1] and [2]), so they do not require manual modification.

[1] https://searchfox.org/mozilla-central/rev/799c3f49233257bd5e899c4107f65ea394b7ae96/dom/interfaces/base/nsIDOMWindowUtils.idl#1599
[2] https://searchfox.org/mozilla-central/rev/799c3f49233257bd5e899c4107f65ea394b7ae96/layout/base/nsIPresShell.h#1811
Flags: needinfo?(botond)
Sounds great to me, I have already changed them. 

I'll presubmit a patch now, but I haven't found relevant gtests to run so I'm not really sure I'm there just yet. However the application builds correctly.
Btw I have just noticed a couple of clang-format related nits, I'll fix them tomorrow :)
Thanks for the patch!

(In reply to Jeremy Lempereur [:o0ignition0o] from comment #12)
> Btw I have just noticed a couple of clang-format related nits, I'll fix them
> tomorrow :)

Yeah, there are a number of unrelated formatting changes in the patch. I'll wait for you to upload a version without them, as they add noise to the diff.
Comment on attachment 8994046 [details]
Bug 1471708 - Rename the "scroll-position clamping scroll port size" to "visual viewport size".

https://reviewboard.mozilla.org/r/258628/#review265880

Clearing review flag until a version withut formatting changes is uploaded.
Attachment #8994046 - Flags: review?(botond)
Ok I just prepared an other patch, but I somehow can't compile anymore => 

> 37:33.81 mozilla-unified/gfx/skia/skia/src/gpu/GrRenderTargetContext.cpp(90): warning C4005: 'ASSERT_SINGLE_OWNER': macro redefinition
> 37:33.81 mozilla/mozilla-unified/gfx/skia/skia/src/gpu/GrProxyProvider.cpp(30): note: see previous definition of 'ASSERT_SINGLE_OWNER'

Not really sure it has something to do with my changes though Oo
A clobber did the trick.

I'm going to run the mochitests tomorrow (compilation takes a while).

By the way I've seen occurrences of "Scroll Port" but I assume it's not in the scope of this bug?

Thanks a lot for your patience :)
Comment on attachment 8994046 [details]
Bug 1471708 - Rename the "scroll-position clamping scroll port size" to "visual viewport size".

https://reviewboard.mozilla.org/r/258628/#review266230

Elsewhere in the code, as well as in the Visual Viewport API spec [1], the word "viewport" is one word, and the "p" is not capitalized in identifier names. It would be good to remain consistent with that in this patch.

Otherwise looks pretty good, thanks!

[1] https://wicg.github.io/visual-viewport/index.html

::: layout/base/nsLayoutUtils.cpp:9053
(Diff revision 2)
>      nsPresContext::CSSPixelsToAppUnits(aSize.width),
>      nsPresContext::CSSPixelsToAppUnits(aSize.height));
>  
>    // When the "font.size.inflation.minTwips" preference is set, the
>    // layout depends on the size of the screen.  Since when the size
>    // of the screen changes, the scroll position clamping scroll port

"the visual viewport size changes"

::: layout/generic/ViewportFrame.cpp:284
(Diff revision 2)
>    NS_ASSERTION(GetAbsoluteContainingBlock()->GetChildList().IsEmpty() ||
>                 (offset.x == 0 && offset.y == 0),
>                 "We don't handle correct positioning of fixed frames with "
>                 "scrollbars in odd positions");
>  
> -  // If a scroll position clamping scroll-port size has been set, layout
> +  // If a scroll position clamping view port size has been set, layout

"If a visual viewport size has been set, ..."

::: layout/painting/nsDisplayList.h:1469
(Diff revision 2)
>  
>        if (nsLayoutUtils::IsFixedPosFrameInDisplayPort(aFrame) &&
>            aBuilder->IsPaintingToWindow()) {
>          // position: fixed items are reflowed into and only drawn inside the
> -        // viewport, or the scroll position clamping scrollport size, if one is
> +        // viewport, or the visual view port size, if one is
>          // set.

This can move up to the previous line.
Attachment #8994046 - Flags: review?(botond)
(In reply to Jeremy Lempereur [:o0ignition0o] from comment #17)
> I'm going to run the mochitests tomorrow (compilation takes a while).

There's no need to run mochitests locally. For a change like this, verifying that it builds locally is sufficient. We can then push the patch to our Try server, where mochitest and other test suites will run in the cloud, before committing the patch.

> By the way I've seen occurrences of "Scroll Port" but I assume it's not in
> the scope of this bug?

Yep, the "scroll port" and the "scroll position clamping scroll port" are two different things, and we're only renaming the latter.
Comment on attachment 8994046 [details]
Bug 1471708 - Rename the "scroll-position clamping scroll port size" to "visual viewport size".

https://reviewboard.mozilla.org/r/258628/#review266488

There is also a usage in the reftest harness:

https://searchfox.org/mozilla-central/rev/bdfd20ef30d521b57d5b6feeda71325e8b4cad66/layout/tools/reftest/reftest-content.js#298
It must start looking better now.

I've also noticed I missed a couple of scroll port sizes in nsGfxScrollFrame.cpp, which are now fixed :)
Comment on attachment 8994046 [details]
Bug 1471708 - Rename the "scroll-position clamping scroll port size" to "visual viewport size".

https://reviewboard.mozilla.org/r/258628/#review267622

Thanks! This is indeed looking better. Just a few minor comments remaining:

::: layout/generic/nsGfxScrollFrame.cpp:2854
(Diff revisions 2 - 3)
>  
>    bool needFrameVisibilityUpdate = mLastUpdateFramesPos == nsPoint(-1,-1);
>  
>    nsPoint dist(std::abs(pt.x - mLastUpdateFramesPos.x),
>                 std::abs(pt.y - mLastUpdateFramesPos.y));
> -  nsSize scrollPortSize = GetVisualViewPortSize();
> +  nsSize viewPortSize = GetVisualViewportSize();

I'm a bit uncertain about this variable rename. "Viewport" has a meaning in layout code that's distinct from "visual viewport". I think we should either leave the variable as |scrollPortSize|, or rename it to |visualViewportSize| (note also the capitalization).

::: layout/generic/nsGfxScrollFrame.cpp:4050
(Diff revisions 2 - 3)
>  {
>    if (!ShouldClampScrollPosition()) {
>      return nsRect(nscoord_MIN/2, nscoord_MIN/2,
>                    nscoord_MAX - nscoord_MIN/2, nscoord_MAX - nscoord_MIN/2);
>    }
> -  nsSize scrollPortSize = GetVisualViewPortSize();
> +  nsSize viewPortSize = GetVisualViewportSize();

Likewise here, keep it |scrollPortSize| or |visualViewportSize|.

::: layout/generic/nsGfxScrollFrame.cpp:6103
(Diff revisions 2 - 3)
>    }
>  
>    // For that, we first convert the scroll port and the scrolled rect to rects
>    // relative to the reference frame, since that's the space where painting does
>    // snapping.
> -  nsSize scrollPortSize = GetVisualViewPortSize();
> +  nsSize viewPortSize = GetVisualViewportSize();

Same here.

::: layout/generic/nsGfxScrollFrame.cpp:6418
(Diff revisions 2 - 3)
>    }
>  
>    result.mScrollSnapTypeX = styles.mScrollSnapTypeX;
>    result.mScrollSnapTypeY = styles.mScrollSnapTypeY;
>  
> -  nsSize scrollPortSize = aScrollFrame.GetScrollPortRect().Size();
> +  nsSize viewPortSize = aScrollFrame.GetScrollPortRect().Size();

Here, we're getting the regular scroll port (not the scroll position clamping scroll port), so we should definitely keep this as |scrollPortSize|.
Attachment #8994046 - Flags: review?(botond)
Comment on attachment 8994046 [details]
Bug 1471708 - Rename the "scroll-position clamping scroll port size" to "visual viewport size".

https://reviewboard.mozilla.org/r/258628/#review268454

Looks good, thanks!
Attachment #8994046 - Flags: review?(botond) → review+
I pushed the patch to our Try server to make sure it's building on all platfomrs and passing tests:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50671dc970f645e0d48304c1ea723fa5d15d35fd
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/542243f5f600
Rename the "scroll-position clamping scroll port size" to "visual viewport size". r=botond
Try push looks good so I went ahead and committed the patch to mozilla-inbound. It should merge over to mozilla-central within a day or so.
Backed out for failling reftest on gfx/layers/apz/test/reftest/async-scrollbar-1.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=542243f5f600243e088c949e647690c8c46fa5b0&tochange=5c5509a1a351d8ccba033b8702a84e7e2b0dedd8&filter-searchStr=reftest&selectedJob=192107015

Failure log: 
https://treeherder.mozilla.org/logviewer.html#?job_id=192107015&repo=autoland&lineNumber=3959
https://treeherder.mozilla.org/logviewer.html#?job_id=192107683&repo=autoland&lineNumber=2477
https://treeherder.mozilla.org/logviewer.html#?job_id=192106961&repo=autoland&lineNumber=1661

Backout link: 
[task 2018-08-05T03:10:42.310Z] 03:10:42     INFO -  TEST-INFO | screentopng: exit 0
[task 2018-08-05T03:10:56.962Z] 03:10:56  WARNING -  TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_group_zoom.html | application timed out after 370 seconds with no output
[task 2018-08-05T03:10:56.962Z] 03:10:56     INFO -  INFO | automation.py | Application ran for: 0:14:49.822610
[task 2018-08-05T03:10:56.963Z] 03:10:56     INFO -  INFO | zombiecheck | Reading PID log: /tmp/tmp96FwDQpidlog
[task 2018-08-05T03:10:57.276Z] 03:10:57     INFO -  Contents of /data/anr/traces.txt:
[task 2018-08-05T03:10:57.277Z] 03:10:57     INFO -  ----- pid 1373 at 2018-08-04 20:10:42 -----
[task 2018-08-05T03:10:57.277Z] 03:10:57     INFO -  Cmd line: org.mozilla.fennec_aurora
[task 2018-08-05T03:10:57.278Z] 03:10:57     INFO -  JNI: CheckJNI is on; workarounds are off; pins=0; globals=257
[task 2018-08-05T03:10:57.278Z] 03:10:57     INFO -  DALVIK THREADS:
[task 2018-08-05T03:10:57.279Z] 03:10:57     INFO -  (mutexes: tll=0 tsl=0 tscl=0 ghl=0)
[task 2018-08-05T03:10:57.282Z] 03:10:57     INFO -  "main" prio=5 tid=1 NATIVE
[task 2018-08-05T03:10:57.282Z] 03:10:57     INFO -    | group="main" sCount=1 dsCount=0 obj=0x414c9578 self=0x2a00d090
[task 2018-08-05T03:10:57.282Z] 03:10:57     INFO -    | sysTid=1373 nice=0 sched=0/0 cgrp=apps handle=1073811452
[task 2018-08-05T03:10:57.282Z] 03:10:57     INFO -    | state=S schedstat=( 85772880369 78446099799 95685 ) utm=7062 stm=1515 core=0
[task 2018-08-05T03:10:57.282Z] 03:10:57     INFO -    #00  pc 0001c5a4  /system/lib/libc.so (__futex_syscall3+8)
[task 2018-08-05T03:10:57.282Z] 03:10:57     INFO -    #01  pc 0000e688  /system/lib/libc.so (__pthread_cond_timedwait_relative+48)
[task 2018-08-05T03:10:57.282Z] 03:10:57     INFO -    #02  pc 0000e6e8  /system/lib/libc.so (__pthread_cond_timedwait+64)
[task 2018-08-05T03:10:57.282Z] 03:10:57     INFO -    #03  pc 00052e97  /system/lib/libdvm.so
[task 2018-08-05T03:10:57.282Z] 03:10:57     INFO -    #04  pc 00053461  /system/lib/libdvm.so (dvmChangeStatus(Thread*, ThreadStatus)+30)
[task 2018-08-05T03:10:57.282Z] 03:10:57     INFO -    #05  pc 00048d29  /system/lib/libdvm.so
[task 2018-08-05T03:10:57.282Z] 03:10:57     INFO -    #06  pc 00039afd  /system/lib/libdvm.so
[task 2018-08-05T03:10:57.282Z] 03:10:57     INFO -    #07  pc 0004b7a9  /system/lib/libandroid_runtime.so (android::AndroidRuntime::getJNIEnv()+16)
[task 2018-08-05T03:10:57.282Z] 03:10:57     INFO -    #08  pc 0005eed5  /system/lib/libandroid_runtime.so (android::NativeDisplayEventReceiver::dispatchVsync(long long, int, unsigned int)+20)
[task 2018-08-05T03:10:57.283Z] 03:10:57     INFO -    #09  pc 0005f0b9  /system/lib/libandroid_runtime.so (android::NativeDisplayEventReceiver::handleEvent(int, int, void*)+80)
[task 2018-08-05T03:10:57.283Z] 03:10:57     INFO -    #10  pc 00015129  /system/lib/libutils.so (android::Looper::pollInner(int)+468)
[task 2018-08-05T03:10:57.284Z] 03:10:57     INFO -    #11  pc 000151d5  /system/lib/libutils.so (android::Looper::pollOnce(int, int*, int*, void**)+92)
[task 2018-08-05T03:10:57.284Z] 03:10:57     INFO -    #12  pc 00067b69  /system/lib/libandroid_runtime.so (android::NativeMessageQueue::pollOnce(_JNIEnv*, int)+22)
[task 2018-08-05T03:10:57.284Z] 03:10:57     INFO -    #13  pc 0001dc4c  /system/lib/libdvm.so (dvmPlatformInvoke+112)
[task 2018-08-05T03:10:57.285Z] 03:10:57     INFO -    #14  pc 0004dcab  /system/lib/libdvm.so (dvmCallJNIMethod(unsigned int const*, JValue*, Method const*, Thread*)+394)
[task 2018-08-05T03:10:57.285Z] 03:10:57     INFO -    #15  pc 000385e1  /system/lib/libdvm.so (dvmCheckCallJNIMethod(unsigned int const*, JValue*, Method const*, Thread*)+8)
[task 2018-08-05T03:10:57.285Z] 03:10:57     INFO -    #16  pc 00027060  /system/lib/libdvm.so
[task 2018-08-05T03:10:57.286Z] 03:10:57     INFO -    #17  pc 0002b580  /system/lib/libdvm.so (dvmInterpret(Thread*, Method const*, JValue*)+184)
[task 2018-08-05T03:10:57.286Z] 03:10:57     INFO -    #18  pc 0005ff7b  /system/lib/libdvm.so (dvmInvokeMethod(Object*, Method const*, ArrayObject*, ArrayObject*, ClassObject*, bool)+350)
[task 2018-08-05T03:10:57.287Z] 03:10:57     INFO -    #19  pc 00067a9f  /system/lib/libdvm.so
[task 2018-08-05T03:10:57.287Z] 03:10:57     INFO -    #20  pc 00027060  /system/lib/libdvm.so
[task 2018-08-05T03:10:57.287Z] 03:10:57     INFO -    #21  pc 0002b580  /system/lib/libdvm.so (dvmInterpret(Thread*, Method const*, JValue*)+184)
[task 2018-08-05T03:10:57.288Z] 03:10:57     INFO -    #22  pc 0005fcbd  /system/lib/libdvm.so (dvmCallMethodV(Thread*, Method const*, Object*, bool, JValue*, std::__va_list)+292)
[task 2018-08-05T03:10:57.288Z] 03:10:57     INFO -    #23  pc 000499ab  /system/lib/libdvm.so
[task 2018-08-05T03:10:57.288Z] 03:10:57     INFO -    #24  pc 0003cb3d  /system/lib/libdvm.so
[task 2018-08-05T03:10:57.289Z] 03:10:57     INFO -    #25  pc 0004b68f  /system/lib/libandroid_runtime.so
[task 2018-08-05T03:10:57.289Z] 03:10:57     INFO -    #26  pc 0004c30f  /system/lib/libandroid_runtime.so (android::AndroidRuntime::start(char const*, char const*)+378)
[task 2018-08-05T03:10:57.289Z] 03:10:57     INFO -    #27  pc 0000105b  /system/bin/app_process
[task 2018-08-05T03:10:57.290Z] 03:10:57     INFO -    #28  pc 0000db4f  /system/lib/libc.so (__libc_init+50)
[task 2018-08-05T03:10:57.290Z] 03:10:57     INFO -    #29  pc 00000d7c  /system/bin/app_process
Flags: needinfo?(jeremy.lempereur)
I'm gonna rebuild it and try to figure out what happened
Flags: needinfo?(jeremy.lempereur)
Comment on attachment 8994046 [details]
Bug 1471708 - Rename the "scroll-position clamping scroll port size" to "visual viewport size".

https://reviewboard.mozilla.org/r/258628/#review268482

::: layout/generic/nsGfxScrollFrame.cpp
(Diff revision 4)
>    aState->mInsideBorderSize =
>      ComputeInsideBorderSize(aState, desiredInsideBorderSize);
> -  nsSize scrollPortSize = nsSize(std::max(0, aState->mInsideBorderSize.width - vScrollbarDesiredWidth),
> +  nsSize visualViewportSize = nsSize(std::max(0, aState->mInsideBorderSize.width - vScrollbarDesiredWidth),
>                                   std::max(0, aState->mInsideBorderSize.height - hScrollbarDesiredHeight));
>  
> -  nsSize visualScrollPortSize = scrollPortSize;

The removal of this variable copy is suspicious. We were copying this variable for a reason: we make modifications to one copy (inside the 'if' block just below) that we don't make to the other copy.

By removing the copy, you are causing that modification to be made to the other copy as well.

This may be the cause of the test failure.
It seems legit, I'm too used of immutability by default ^^, I'll push the changes today.
Keywords: checkin-needed
Marking it as checkin-needed, feel free to cancel it if the change is not up to your standard or if I shall change something else :)
You still have one open issue, please take a look.
Flags: needinfo?(jeremy.lempereur)
Keywords: checkin-needed
Hi dvarga :) 

I'm sorry I actually pushed changes but didn't click on "fixed".

Do you think I can mark it as fixed and put the flag back ?
Flags: needinfo?(jeremy.lempereur)
Sorry, you should get a review.
This would indeed be wiser, I ll wait :)
Comment on attachment 8994046 [details]
Bug 1471708 - Rename the "scroll-position clamping scroll port size" to "visual viewport size".

https://reviewboard.mozilla.org/r/258628/#review268580

::: layout/generic/nsGfxScrollFrame.cpp:387
(Diff revisions 4 - 5)
>    aState->mInsideBorderSize =
>      ComputeInsideBorderSize(aState, desiredInsideBorderSize);
> -  nsSize visualViewportSize = nsSize(std::max(0, aState->mInsideBorderSize.width - vScrollbarDesiredWidth),
> +  nsSize scrollPortSize = nsSize(std::max(0, aState->mInsideBorderSize.width - vScrollbarDesiredWidth),
>                                   std::max(0, aState->mInsideBorderSize.height - hScrollbarDesiredHeight));
>  
> +  nsSize visualViewportSize = scrollPortSize;

This doesn't fix the issue.

There were uses of |scrollPortSize| below the if statement which were expecting to see the original value (without the modification made to |visualViewportSize| inside the if statement), but now see the modified value because they have been renamed to |visualViewportSize|.

If you look at the complete diff for this function, there were two variables: |scrollPortSize|, and |visualScrollPortSize|. It's fine to rename |visualScrollPortSize| to |visualViewportSize|, but only uses of |visualScrollPortSize| should be changed to |visualViewportSize|, not uses of |scrollPortSize|.
Attachment #8994046 - Flags: review+
And yes, the fact that MozReview carries a review+ across a backout is a process hole :)
Good to know, thanks for your patience, I'll have a more thorough look at the diff, and submit a patch tomorrow !
Comment on attachment 8994046 [details]
Bug 1471708 - Rename the "scroll-position clamping scroll port size" to "visual viewport size".

https://reviewboard.mozilla.org/r/258628/#review268644

Thanks, this looks good!

Here's a new Try push, this time including the Android tests that failed last time:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3f953b20d81a912bb269a486a17a8ba8b7f5aa6
Attachment #8994046 - Flags: review?(botond) → review+
The new Try push is looking good, so I went ahead and re-landed the patch.
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/69862af7f7d2
Rename the "scroll-position clamping scroll port size" to "visual viewport size". r=botond
https://hg.mozilla.org/mozilla-central/rev/69862af7f7d2
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Blocks: 1488969
Blocks: 1489910
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: