Closed Bug 1194851 Opened 4 years ago Closed 4 years ago

[RTL] [APZ] Fake-scrollbar-space is drawn in scrollable things when APZ enabled

Categories

(Core :: Panning and Zooming, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla46
Tracking Status
firefox46 --- verified

People

(Reporter: alice0775, Assigned: tnikkel)

References

Details

(Keywords: regression, rtl)

Attachments

(15 files, 4 obsolete files)

1.45 MB, video/ogg
Details
187 bytes, text/html
Details
51.94 KB, image/png
Details
606 bytes, text/html
Details
58 bytes, text/x-review-board-request
kats
: review+
Details
58 bytes, text/x-review-board-request
kats
: review+
Details
1.16 KB, patch
botond
: review+
Details | Diff | Splinter Review
5.57 KB, patch
botond
: review+
Details | Diff | Splinter Review
8.55 KB, patch
botond
: review+
Details | Diff | Splinter Review
1.62 KB, patch
botond
: review+
Details | Diff | Splinter Review
5.07 KB, patch
botond
: review+
Details | Diff | Splinter Review
5.97 KB, patch
botond
: review+
Details | Diff | Splinter Review
9.08 KB, patch
botond
: review+
Details | Diff | Splinter Review
4.93 KB, patch
botond
: review+
Details | Diff | Splinter Review
10.59 KB, patch
botond
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #1194346 +++


Steps To Reproduce
1. Start RTL language Firefox(Hebrew version) and Enable APZ(default in Nightly42.0a1).
2. Create about:config pref "dom.allow_XUL_XBL_for_file", set to true, so that you can load XUL directly.
3. Download & load attachment 8648184 [details] testcase.
4. (optional) Reduce the width of your Firefox window.
5. (optional) reload or scroll the listbox.

Actual Results:
 Fake-scrollbar-space is drawn at the right side of the listbox

Expected Results:
 No such fake-scrollbar-space
(In reply to Alice0775 White from comment #0)
> 1. Start RTL language Firefox(Hebrew version) and Enable APZ(default in
> Nightly42.0a1).

I can reproduce this in a standard Firefox Nightly version (doesn't have to be Hebrew).

(And note that on Linux nightlies, APZ is not yet on by the default - you have to turn on pref "layers.async-pan-zoom.enabled" and restart Firefox. With that, I can reproduce this bug.)
I can actually reproduce this with a <textarea>, too -- no need for XUL. Testcase coming up, & reclassifying.
Component: XUL → Panning and Zooming
Summary: [RTL] [APZ] Fake-scrollbar-space is drawn in XUL listbox when APZ enabled → [RTL] [APZ] Fake-scrollbar-space is drawn in scrollable things when APZ enabled
Here's a screenshot comparing an APZ-enabled profile (top) to an APZ-disabled profile [default on linux], (bottom), viewing the testcase I just attached.

Note the difference in the right edge of the textboxes.
This is not a clipping issue as I originally suspected; it's that the displayport is truncated. The displayport computation involves taking the displayport base rect, doing a bunch of stuff to expand it, and then clamping it to the scrollable rect size. The base rect has an x>0 because of the scrollbar being on the left. The scrollable rect has an x=0. And then then two are intersected, resulting in the truncated displayport width.

Fundamentally it's a coordinate space mismatch but I'm not sure which coordinate space is the "correct" one to be working in here. Having a base rect with x=0 would in theory give the correct displayport result but it might be more conceptually correct to shift the scrollable rect and un-shift the displayport after the intersection.
Assignee: nobody → bugmail.mozilla
Actually there's other stuff I should work on first.
Assignee: bugmail.mozilla → nobody
Bug 1194851 - Don't clip scrolled contents with display ports if the scrollport doesn't start at (0,0). r?kats
Attachment #8655687 - Flags: review?(bugmail.mozilla)
Duplicate of this bug: 1190105
I'm not really sure whether that's the right thing to do, or whether the comment is correct. It seems to work as expected, though. For example I'm not suddenly clipping away stuff in the top left corner.
Comment on attachment 8655687 [details]
MozReview Request: Bug 1194851 - Don't clip scrolled contents with display ports if the scrollport doesn't start at (0,0). r?kats

I'm probably not the right person to review this; tn knows this code much better. Redirecting review to him.
Attachment #8655687 - Flags: review?(bugmail.mozilla) → review?(tnikkel)
Comment on attachment 8655687 [details]
MozReview Request: Bug 1194851 - Don't clip scrolled contents with display ports if the scrollport doesn't start at (0,0). r?kats

https://reviewboard.mozilla.org/r/17985/#review16141

Good catch.

I feel like this mistake is probably being made in several other places in our code, we should audit.
Attachment #8655687 - Flags: review?(tnikkel) → review+
Hmm, actually the displayport base is the original dirty rect, so it will be relative to mOuter as well. And we just expand that by the display port margins. So shouldn't the display port be relative to mOuter then as well?
Comment on attachment 8655687 [details]
MozReview Request: Bug 1194851 - Don't clip scrolled contents with display ports if the scrollport doesn't start at (0,0). r?kats

https://reviewboard.mozilla.org/r/17985/#review16143
Attachment #8655687 - Flags: review+
Looks like the problem is in GetDisplayPortFromMarginsData http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=359809ae3db3#1004 when we intersect the display port with the expanded scrollable rect. Up to that point the base rect and hence the displayport is relative to the scrollframe, but the expanded scrollable rect is relative to the scrollport.
And I take it the adjustment by scrollPos we do on the next line is insufficient to compensate for this? What other component are we missing there?
Flags: needinfo?(tnikkel)
Duplicate of this bug: 1206032
No longer responding. How about?
Assignee: nobody → tnikkel
Flags: needinfo?(tnikkel)
I can reproduce this bug with an HTML listbox (<select size="...">).  Specifically, when I view the attached testcase in Nightly with a fresh profile, the 3rd digit is missing in each line of the RTL listbox.

(I can fix this by disabling e10s in Firefox Preferences, or by disabling layers.async-pan-zoom.enabled.)
Priority: -- → P1
Duplicate of this bug: 1193781
Copying over regression properties from bug 1193781 comment 2.
Blocks: 1126090
Keywords: regression
I'm going to look at this while Timothy is away.
Assignee: tnikkel → botond
^ These are some preliminary patches that make it easier to audit the places in the code that actually consume the value of the displayport.
Based on my conversation with Kats today and some investigation, here's my summary of the problem:

  - Depending on where it's used, the displayport is interpreted as being relative to
    either the scroll frame, or the scroll port.

  - The scroll port's position relative to the scroll frame is frequently (0, 0)
    (which is why this has gone unnoticed for so long), but it can be nonzero if
    there are borders involved or there's a non-overlay scrollbar in an RTL setup.

Auditing the places that actually consume the value of the displayport:

  (a) nsLayoutUtils::ComputeFrameMetrics         [1]
  (b) ScrollFrameHelper::DecideScrollableLayer   [2]
  (c) ScrollFrameHelper::IsRectNearlyVisible     [3]
  (d) ScrollFrameHelper::ScrollToImpl            [4] [5]
  (e) nsDisplaySubDocument::ComputeVisibility    [6]
  (f) PresShell::MarkImagesInSubtreeVisible      [7]
  (g) ContainerState::ComputeOpaqueRect          [8]

[1] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/base/nsLayoutUtils.cpp#8441
[2] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/generic/nsGfxScrollFrame.cpp#3204
[3] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/generic/nsGfxScrollFrame.cpp#3344
[4] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/generic/nsGfxScrollFrame.cpp#2524
[5] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/generic/nsGfxScrollFrame.cpp#2540
[6] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/base/nsDisplayList.cpp#4500
[7] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/base/nsPresShell.cpp#5677
[8] https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/layout/base/FrameLayerBuilder.cpp#3736

  (a) Ships the displayport off to the compositor, which interpets it as being
      relative to the scroll port.

  (b), (c), (e), (f), and (g) interpret the displayport as being relative to
      the scroll frame (as far as I can tell).

  (d) Doesn't care because it just cares about whether the displayport has changed.

So this is what the displayport is _expected_ to be relative to.

As Timothy points out in comment 15, what the value of the displayport is _actually_ relative to is something nonsensical, because the value is arrived at in GetDisplayPortFromMarginsData() by intersecting a rectangle relative to the scroll port (the expanded scrollable rect) with a rectangle relative to the scroll frame (the display port base inflated by the margins).

Given this, here's my proposed fix approach:

  - Fix GetDisplayPortFromMarginsData() by translating the expanded scrollable rect
    to be relative to the scroll frame (by adding to it the position of the scroll
    port relative to the scroll frame) prior to the intersection. This ensures that
    the resulting rect is properly relative to the scroll frame.

  - As most consumers either expect the displayport to be relative to the scroll
    frame or don't care, this means most consumers don't need to be changed.

  - The one consumer that expects the displayport to be relative to the scroll port,
    ComputeFrameMetrics(), can translate it back to be thus.

Thoughts?
No longer blocks: 1193781
Attachment #8699247 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8699247 [details]
MozReview Request: Bug 1194851 - Add helper functions HasDisplayPort() and HasCriticalDisplayPort(). r=kats

https://reviewboard.mozilla.org/r/28209/#review25295

Nice cleanup, thanks!

::: layout/base/nsLayoutUtils.cpp:1082
(Diff revision 1)
> +nsLayoutUtils::HasDisplayPort(nsIContent* aContent) {

nit: brace on newline
Comment on attachment 8699248 [details]
MozReview Request: Bug 1194851 - Remove the second parameter of IsFixedPosFrameInDisplayPort(). r=kats

https://reviewboard.mozilla.org/r/28211/#review25297
Attachment #8699248 - Flags: review?(bugmail.mozilla) → review+
(In reply to Botond Ballo [:botond] from comment #26)
> Based on my conversation with Kats today and some investigation, here's my
> summary of the problem:

Thanks for the detailed analysis! Documenting this here makes it really easy to follow :)

> Given this, here's my proposed fix approach:
> 
>   - Fix GetDisplayPortFromMarginsData() by translating the expanded
> scrollable rect
>     to be relative to the scroll frame (by adding to it the position of the
> scroll
>     port relative to the scroll frame) prior to the intersection. This
> ensures that
>     the resulting rect is properly relative to the scroll frame.
> 
>   - As most consumers either expect the displayport to be relative to the
> scroll
>     frame or don't care, this means most consumers don't need to be changed.
> 
>   - The one consumer that expects the displayport to be relative to the
> scroll port,
>     ComputeFrameMetrics(), can translate it back to be thus.
> 
> Thoughts?

Assuming the conversion back and forth is lossless, this sounds fine to me. My impression from talking to Timothy was that he was going to make the displayport getter take an extra parameter that would return it in one of the two spaces, but that adds more complexity so I prefer this approach.
(In reply to Botond Ballo [:botond] from comment #26)
>   (a) Ships the displayport off to the compositor, which interpets it as
> being
>       relative to the scroll port.

Ah, this is what I needed to know. I looked at some of the uses of displayport on the frame metrics, and they were relative to the scrollport, but didn't get to look at them all.

I was planning to finish this off before I came here to see the progress, since it looks like you are on pto/holidays I'll steal this back, unless there are objections.
(In reply to Timothy Nikkel (:tn) from comment #30)
> I was planning to finish this off before I came here to see the progress,
> since it looks like you are on pto/holidays I'll steal this back, unless
> there are objections.

No objection - thanks!
Two new things I've found:
-the coordinate system of the display port base isn't consistent. APZC callback helper sets them to the composition bounds (ie relative to the scroll port), layout does relative to scrollframe.
-the uses of displayport might be heavily weighted towards wanting them relative to the scrollport, but the uses of the nsLayoutUtils::GetDisplayPort itself mostly want it relative to the scroll frame. I think the count was 7 (scrollframe) to 2 (scrollport). So in the interest of not repeating the adjustment code everywhere having a getter GetDisplayPortRelativeToScrollFrame I think is a good idea. This gets more complicated because we have GetOrMaybeCreateDisplayPort, ViewportHasDisplayPort that also want the displayport relative the scrollframe, but who's name might make it hard to make this distinction clear.
I landed the small refactoring patches posted in comment 23 and comment 24. Marking bug as leave-open so that the bug doesn't get closed when these merge to m-c.

Timothy is actively working on the actual fix, based on the plan described in comment 32; updating assignee to reflect this.
Assignee: botond → tnikkel
Keywords: leave-open
Attachment #8655687 - Attachment is obsolete: true
Feel free to pass any review onto Markus if you would prefer.
Attachment #8704060 - Flags: review?(botond)
Attachment #8704062 - Flags: review?(botond)
Keywords: leave-open
Attachment #8704062 - Attachment is obsolete: true
Attachment #8704062 - Flags: review?(botond)
Attachment #8704584 - Flags: review?(botond)
Attachment #8704064 - Attachment is obsolete: true
Attachment #8704064 - Flags: review?(botond)
Attachment #8704588 - Flags: review?(botond)
Attachment #8704056 - Flags: review?(botond) → review+
Comment on attachment 8704057 [details] [diff] [review]
Part 4. Make ViewportHasDisplayPort only return a bool

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

I guess both in this patch and the next one, we're making a tradeoff of having an extra GetProperty() call (one when checking if the displayport exists, and the second when getting it) to get a simpler interface?

::: layout/base/nsLayoutUtils.h
@@ +526,5 @@
>                                          bool aIsClipFixed);
>  
>    /**
>     * Return true if aPresContext's viewport has a displayport.
>     * Fills in aDisplayPort with the displayport rectangle if non-null.

Update comment
Attachment #8704057 - Flags: review?(botond) → review+
Attachment #8704060 - Flags: review?(botond) → review+
Comment on attachment 8704061 [details] [diff] [review]
Part 6. Store the displayport base relative to the scrollport.

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +3235,5 @@
> +    } else {
> +      // Restrict the dirty rect to the scrollport, and make it relative to the
> +      // scrollport for the displayport base.
> +      displayportBase = aDirtyRect->Intersect(mScrollPort);
> +      displayportBase -= mScrollPort.TopLeft();

All added lines should be intended by one more level
Attachment #8704061 - Flags: review?(botond) → review+
Comment on attachment 8704584 [details] [diff] [review]
Part 7. Create GetDisplayPortRelativeToScrollFrame

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

::: layout/base/nsLayoutUtils.cpp
@@ +850,5 @@
>    return ApplyRectMultiplier(aRectData->mRect, aMultiplier);
>  }
>  
> +nsIFrame*
> +GetScrollFrameFromContent(nsIContent* aContent)

Can we refactor nsLayoutUtils::FindScrollableFrameFor to use this?

@@ +1085,5 @@
>    return GetDisplayPortImpl(aContent, aResult, 1.0f);
>  }
>  
>  bool
> +nsLayoutUtils::GetDisplayPortRelativeToScrollFrame(nsIContent* aContent, nsRect *aResult)

nsRect* aResult
Attachment #8704584 - Flags: review?(botond) → review+
Comment on attachment 8704585 [details] [diff] [review]
Part 8. Change GetDisplayPortForVisibilityTesting

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

::: layout/base/nsLayoutUtils.h
@@ +184,5 @@
>    /**
>     * @return the display port for the given element which should be used for
>     * visibility testing purposes.
>     *
>     * If low-precision buffers are enabled, this is the critical display port;

"the critical display port relative to the scroll frame"
Attachment #8704585 - Flags: review?(botond) → review+
Comment on attachment 8704587 [details] [diff] [review]
Part 9. Temporarily rename GetDisplayPort to GetDisplayPortRelativeToScrollPort and convert all GetDisplayPort callers.

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

One possible way the final API could look is:

  enum class RelativeTo {
    ScrollPort,
    ScrollFrame
  };

  bool GetDisplayPort(nsIContent* aContent, nsRect* aResult, RelativeTo aRelativeTo = RelativeTo::ScrollPort) {
    // get it relative to the scroll port
    if (aRelativeTo == RelativeTo::ScrollFrame) {
      // translate it
    }
  }

I'm fine with the API these patches put in place, too; just throwing out a possible alternative idea.
Attachment #8704587 - Flags: review?(botond) → review+
Attachment #8704588 - Flags: review?(botond) → review+
Thanks for breaking up this patch set so nicely! It made it easy to review.
(In reply to Botond Ballo [:botond] from comment #51)
> One possible way the final API could look is:

That was actually my first idea, but I think kats said it seemed more complicated, but he might have been meaning something else. So that's why I went a different direction.

I can add a patch on the end of these to change the api to this if we all agree.
Flags: needinfo?(bugmail.mozilla)
Flags: needinfo?(botond)
I liked botond's proposal in comment 26 but I haven't looked at the patches/code in detail so if something else works out better that's fine. I'm happy to leave it to you guys.
Flags: needinfo?(bugmail.mozilla)
The difference between my proposal in comment 26 and what the current patch implements is:

  - In my proposal, there would have been just one API (GetDisplayPort()) which 
    returns the display port relative to the scroll frame, and call sites that 
    want it relative to the scroll port would adjust it themselves.

  - With the current patches, there are two APIs, GetDisplayPort() which returns
    the display port relative to the scroll port, and GetDisplayPortRelativeToScrollFrame()
    which returns the display port relative to the scroll frame.

In retrospect, I think Timothy's approach is actually better, because internally GetDisplayPort() gets the display port relative to the scroll port, so in my approach it would have had to adjust it, and then callers that wanted it relative to the scroll port would have had to un-adjust it. In other words, Timothy's approach avoids the "conversion back and forth" mentioned in comment 29.

My suggestion in comment 51 is just a cosmetic one, where, rather than having the two APIs look like this:

   GetDisplayPort(content, rect)
   GetDisplayPortRelativeToScrollFrame(content, rect)

they would look like this:

   GetDisplayPort(content, rect)
   GetDisplayPort(content, rect, RelativeTo::ScrollFrame)

(with the first one being a shorthand, using a default argument, for

   GetDisplayPort(content, rect, RelativeTo::ScrollPort)

). Timothy, it's up to you whether you'd like to make this cosmetic change or not; you have my r+ either way.
Flags: needinfo?(botond)
I like the enum api. The only part I don't like is that we need to type nsLayoutUtils::RelativeTo everywhere, which makes it a little longer.
(In reply to Timothy Nikkel (:tnikkel) from comment #57)
> I like the enum api. The only part I don't like is that we need to type
> nsLayoutUtils::RelativeTo everywhere, which makes it a little longer.

We could put RelativeTo outside of nsLayoutUtils.
(In reply to Botond Ballo [:botond] from comment #47)
> I guess both in this patch and the next one, we're making a tradeoff of
> having an extra GetProperty() call (one when checking if the displayport
> exists, and the second when getting it) to get a simpler interface?

Yeah, but we GetProperty should be one hashtable lookup, and we do tons and tons of those for every paint, so a drop in the bucket.
(In reply to Botond Ballo [:botond] from comment #59)
> We could put RelativeTo outside of nsLayoutUtils.

Okay, I'll do that. Was worried about polluting the namespace. But I guess it's fine.
Attachment #8705353 - Flags: review?(botond) → review+
(In reply to Timothy Nikkel (:tnikkel) from comment #61)
> (In reply to Botond Ballo [:botond] from comment #59)
> > We could put RelativeTo outside of nsLayoutUtils.
> 
> Okay, I'll do that. Was worried about polluting the namespace. But I guess
> it's fine.

We could even reuse it in the future for things that aren't the displayport and can be relative to various things. (Although then we may need to check for an unsupported value of RelativeTo being passed to a function like GetDisplayPort).
(In reply to Botond Ballo [:botond] from comment #49)
> >  bool
> > +nsLayoutUtils::GetDisplayPortRelativeToScrollFrame(nsIContent* aContent, nsRect *aResult)
> 
> nsRect* aResult

This already had inconsistent style on GetDisplayPort, so I think I'll just leave the style fixes because I kill this function in the last patch of the series and I don't want to do a ton of rebasing for it.
(In reply to Botond Ballo [:botond] from comment #49)
> ::: layout/base/nsLayoutUtils.cpp
> > +nsIFrame*
> > +GetScrollFrameFromContent(nsIContent* aContent)
> 
> Can we refactor nsLayoutUtils::FindScrollableFrameFor to use this?

Good catch. I'll do this in another bug because I have another patch that changes this function that will go in yet another bug.
(In reply to Timothy Nikkel (:tnikkel) from comment #64)
> (In reply to Botond Ballo [:botond] from comment #49)
> > ::: layout/base/nsLayoutUtils.cpp
> > > +nsIFrame*
> > > +GetScrollFrameFromContent(nsIContent* aContent)
> > 
> > Can we refactor nsLayoutUtils::FindScrollableFrameFor to use this?
> 
> Good catch. I'll do this in another bug because I have another patch that
> changes this function that will go in yet another bug.

Bug 1237813.
Flags: qe-verify+
Reproduced with all attached test cases.

Verified as fixed on Firefox 46 beta 9.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.