Closed Bug 1302389 Opened 3 years ago Closed 3 years ago

scroll-frame with vertical writing-mode does not always compute ScrolledRect of its scrollable content correctly

Categories

(Core :: Layout: Block and Inline, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

(Spun off from bug 1301610, as I'm not sure this is the _only_ issue affecting gtalbot's test cases there, although it's at least part of the problem.)

nsLayoutUtils::GetScrolledRect, which is used by ScrollFrameHelper (in nsGfxScrollFrame.cpp) to determine the extent of the content that needs to be scrolled -- and hence the range of the scrollbar controls -- does not handle vertical writing mode properly. In particular, when the inline-flow direction is upwards (against the physical direction, as used by the scrolling controls), it fails to apply similar logic to what it does for right-to-left horizontal content.

This leads to issues such as the following (from bug 1301610 comment 1). Compare the two testcases:

(1)
data:text/html,
    <div style="height:500px;border:1px solid red;overflow:scroll;writing-mode:sideways-rl;">
    <div style="height:100%;border:50px solid yellow;"><div>hello

(2)
data:text/html,
    <div style="height:500px;border:1px solid red;overflow:scroll;writing-mode:sideways-lr;">
    <div style="height:100%;border:50px solid yellow;"><div>hello

In case (1), using sideways-rl mode, the yellow-bordered child div can be scrolled up and down so as to see its full extent, including the borders. But in case (2), with sideways-lr, it's not scrollable at all.
Here's a set of testcases that exercise this code a bit; four of them currently fail, as noted in the manifest, because we fail to compute the right ScrolledRect for the content, and it remains unscrollable.
Attachment #8790653 - Flags: review?(dholbert)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
While looking into this, I realized that ScrollFrameHelper::IsLTR() is really confusing, because normally we use 'LTR' to refer to inline directionality (unicode bidi), but in this case it's actually referring to physical left-to-rightness, whether that is the inline or block axis. This is wrong for one of the callsites (in GetScrolledFrameDir), but because of the naming, that's not at all obvious. This patch renames the method to IsPhysicalLTR() for greater clarity, and updates all callers accordingly. So there's no behavior change here; that will come in the following patches.
Attachment #8790684 - Flags: review?(dholbert)
Now we can refactor that method and provide a variant IsBidiLTR() which returns the inline bidi directionality of the relevant frame; this is actually what GetScrolledFrameDir() should be using. (Without this fix, the following patch would break at least one existing testcase, because of the resulting logical/physical axis confusion.)
Attachment #8790686 - Flags: review?(dholbert)
And finally, the patch that adds vertical writing-mode support to nsLayoutUtils::GetScrolledRect, and thereby fixes the reftests above. This is a bit ugly because the method still takes its parameters in physical coordinates, but fully converting nsGfxScrollFrame to logical coordinates looks like it might be a bridge too far for now. So I've kept this localized to the single utility method.
Attachment #8790688 - Flags: review?(dholbert)
Sigh... the tests here pass for me locally (with the patches), but tryserver says that various small discrepancies show up on the scrollbars, with details depending on the platform:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ed586d0ee86517daf7cda113171ba01b2005bbc

I'll try to reproduce on another system and see if I can improve things.
OK, it appears that the discrepancy in the lengths of scrollbar thumbs is a separate, pre-existing bug (at least, I guess it's a bug rather than expected behavior, though I haven't entirely understood what's behind it yet). It can be reproduced on a current nightly (or release) build, and is unrelated to the patches here.

So I propose to file a separate bug for that, with its own testcase, and annotate the reftests here as fuzzy for now. They still serve as tests for this bug, because of the fat blue border on the inner <div>, which if positioned wrong will result in a much greater difference than the scrollbar discrepancies.
(You should consider changing your workflow to use MozReview for submitting patches -- it takes a little getting used to, but it makes review a bit easier, by e.g. letting the reviewer click to expand context [one thing I wanted to do when skimming one of the patches here and was unable to do]. Its interdiffs & whitespace-change/code-move highlighting is good, too.  And you can use autoland for the final landing & don't have to worry about whether the tree's open, which is nice. :))
(In reply to Jonathan Kew (:jfkthame) from comment #6)
> So I propose to file a separate bug for that, with its own testcase, and
> annotate the reftests here as fuzzy for now.

Sounds good. Please do include a mention of that other bug number in the fuzzy annotations here, so that we have a decent chance of rediscovering & fixing those fuzzy annotations once we fix the other bug.
Comment on attachment 8790684 [details] [diff] [review]
patch 1 - Rename ScrollFrameHelper::IsLTR to IsPhysicalLTR for clarity (no behavior change)

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

r=me on patch 1
Attachment #8790684 - Flags: review?(dholbert) → review+
Comment on attachment 8790686 [details] [diff] [review]
patch 2 - Add ScrollFrameHelper::IsBidiLTR to return the inline-bidi direction, as opposed to physical LTR-ness, and use this in GetScrolledFrameDir()

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

r=me, one nit:

::: layout/generic/nsGfxScrollFrame.cpp
@@ +4923,2 @@
>  {
>    //TODO make bidi code set these from preferences

This comment at the start of the method ("TODO make bidi code set these from preferences") probably doesn't make sense anymore, given the new name/reduced-scope of this method.

Searchfox says this was added in this patch, in 2004 (back when this function was called IsScrollbarOnRight):
http://searchfox.org/mozilla-central/diff/84b0577142904d967c6a713af614a1e3d36326be/layout/generic/nsGfxScrollFrame.cpp#2025

Really, the comment probably hasn't made sense for a long time, particularly after we introduced writing modes and/or removed IBMBIDI guards from this function.  In any case, seems like now is an appropriate time to call it obsolete & remove it.
Attachment #8790686 - Flags: review?(dholbert) → review+
> given the new name/reduced-scope of this method.

(...and of course Splinter only quoted two lines there, and didn't include the method name. Yet another reason to switch to MozReview. :)  Anyway -- to clarify my comment, I was talking about the method that's being renamed to ScrollFrameHelper::GetFrameForDir.)
Comment on attachment 8790688 [details] [diff] [review]
patch 3 - Handle vertical writing mode in nsLayoutUtils::GetScrolledRect()

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

::: layout/base/nsLayoutUtils.cpp
@@ +2120,5 @@
>                                 const nsSize& aScrollPortSize,
>                                 uint8_t aDirection)
>  {
> +  WritingMode wm = aScrolledFrame->GetWritingMode();
> +  // potentially override the frame's dir (for unicode-bidi:plaintext)

Does this comment mean we should include a testcase with unicode-bidi:plaintext? (I don't see that CSS in any of the reftests here.)

@@ +2124,5 @@
> +  // potentially override the frame's dir (for unicode-bidi:plaintext)
> +  wm.SetDirectionFromBidiLevel(aDirection == NS_STYLE_DIRECTION_RTL ? 1 : 0);
> +  bool vertical = wm.IsVertical();
> +  nscoord i1, i2, b1, b2;
> +  if (vertical) {

Could you add comments above each each section of this function (above each top-level "if" check), to explain what each section is doing?

e.g. this first section probably wants something like:
 // Convert aScrolledFrameOverflowArea's upper-left lower-right points into
 // a logical representation, with logical "points" (i1, b1) and (i2, b2).

And then, the following section seems to be doing some clamping of the physically top/left-most BStart edge, or something like that? (not sure)  Please add a comment explaining that.

(And then the final section does something similar for the inline axis, I guess? Again, a high-level comment would be great.)
Attachment #8790688 - Flags: review?(dholbert) → review-
(r- because I don't quite understand what the code is doing yet, and I think the suggested comments would help with that so I can r+ the next version. :))
(In reply to Daniel Holbert [:dholbert] from comment #10)
> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +4923,2 @@
> >  {
> >    //TODO make bidi code set these from preferences
> 
> This comment at the start of the method ("TODO make bidi code set these from
> preferences") probably doesn't make sense anymore, given the new
> name/reduced-scope of this method.
> 
> Searchfox says this was added in this patch, in 2004 (back when this
> function was called IsScrollbarOnRight):

Actually, it goes back further than that, to 2000 (changeset "Added a hook for the Bidi code", no bug number mentioned):
http://searchfox.org/mozilla-central/diff/c19480372efd5b42e123aede6302d0083772297d/layout/html/base/src/nsGfxScrollFrame.cpp#745

The patch in 2004 just pulled it out of nsGfxScrollFrameInner::Layout into the (trivial) helper method.

But anyway, I agree it seems like a good time to kill it.
Depends on: 1302700
(In reply to Daniel Holbert [:dholbert] from comment #12)
> Does this comment mean we should include a testcase with
> unicode-bidi:plaintext? (I don't see that CSS in any of the reftests here.)

We have existing (horizontal, bidi) tests that fail without this; at one stage, I tried to simplify it away and rely purely on aFrame->GetWritingMode(), but that resulted in several test failures.

However, in the course of looking into this and trying to cook up tests, I found that we don't actually implement unicode-bidi:plaintext properly in vertical writing modes. So I'll file a separate bug on that.
(In reply to Jonathan Kew (:jfkthame) from comment #15)
> So I'll file a separate bug on that.

That's now bug 1302734. I don't think it directly blocks or depends on this bug, so I haven't marked it as such; it's a separate issue.
(In reply to Jonathan Kew (:jfkthame) from comment #5)
> Sigh... the tests here pass for me locally (with the patches), but tryserver
> says that various small discrepancies show up on the scrollbars, with
> details depending on the platform:

FWIW, I can definitely reproduce the scrollbar mismatches locally, on Linux (Ubuntu 16.04), e.g. comparing your scrolled-rect-1a.html testcase vs. its reference case in Nightly. (That's one of the tests whose rendering isn't impacted by the fix here, so it's testable in Nightly and would be expected to match if it weren't for this scrollbar issue.)

Do remember to file a followup for that scrollbar issue.
Actually -- I'll bet the reftests would Just Work (serve the same purpose without triggering scrollbar issues) if you just made them use "overflow: hidden" instead of "overflow: scroll".

That would be great for avoiding hacky "fuzzy" annotations, as well as for making the tests more robust & simple. (Still probably worth filing a followup for the scrollbar issues, but the tests here don't need to depend on that bugfix anymore.)

Could you give that a try? (perhaps you already have and it didn't work)
Flags: needinfo?(jfkthame)
(In reply to Daniel Holbert [:dholbert] from comment #17)
> Do remember to file a followup for that scrollbar issue.

Yup, that's bug 1302700.
Flags: needinfo?(jfkthame)
Comment on attachment 8790653 [details] [diff] [review]
Reftests for scroll-frame with vertical writing mode

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

Anyway, the tests patch looks good (though per comment 18, it might want s/scroll/hidden/, and/or some fuzzy annotations for the tests that aren't affected by the fix here).

Also, one nit: consider adding "- patch 0 -" to the commit message, for consistency with your other patches here. (This one just starts with "Bug 1302389:", with no patch number, unlike the other patches here)

r=me with any/all of those changes as you see fit
Attachment #8790653 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #18)
> Could you give that a try? (perhaps you already have and it didn't work)

That would avoid the failures, yes. But it also neuters a test like 2b, where the reference and testcase render the inner element in the same position, but the bug here means that the testcase shows an inactive scrollbar, whereas in the reference it's active.

As the scrollbar issue seems to be related to font/line metrics, however, I think we can largely mitigate it by setting font-size:0 on all the testcases (which is fine, as they don't rely on using any text, just rectangular <div>s and borders). That appears to bring the amount of "fuzz" needed down to almost zero: https://treeherder.mozilla.org/#/jobs?repo=try&revision=80d0c05cec4e.
(In reply to Jonathan Kew (:jfkthame) from comment #21)
> That would avoid the failures, yes. But it also neuters a test like 2b,
> where the reference and testcase render the inner element in the same
> position, but the bug here means that the testcase shows an inactive
> scrollbar, whereas in the reference it's active.

Oh, good point. It seems like you could still test that we have non-trivial scrollable area in that sort of scenario, if you modify .scrollTop and verify that this does indeed change the rendering (which ensures that it's scrollable, without needing to show the scrollbar).  But, that might require another reference case perhaps, and maybe that makes this more complex.

Good to hear that font-size improves things.
I've re-worked this so that it continues to work with physical coordinates, rather than re-labelling them with 'i' and 'b' as in the previous version (although they were not true 'logical' coordinates there, making things rather confusing). I think this makes it easier to follow, and it becomes clear that this is simply retaining the existing behavior in the horizontal dimension (extended to recognize the horizontal-progression direction of vertical-lr/vertical-rl modes), and adding analogous handling for the vertical dimension (not previously needed, because prior to vertical writing modes, nothing ever progress upwards 'against' the y-coord direction). I'll re-check fuzz figures once all the try results are ready, but as of now it looks like we're down to a virtually imperceptible difference in antialiasing on some of the scrollbars on Linux and OS X. The larger fuzz value on Android is because the use of dir=rtl appears to cause the overlay-style scroll indicator to shift horizontally; that may also be a bug, but it's a separate issue from the ScrolledRect bug here.
Attachment #8791430 - Flags: review?(dholbert)
Attachment #8790688 - Attachment is obsolete: true
Comment on attachment 8791430 [details] [diff] [review]
patch 3 - Handle vertical writing mode in nsLayoutUtils::GetScrolledRect()

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

Thanks! This is indeed easier to follow.

r=me
Attachment #8791430 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d34557a4c1340b77ca46f3794bb091e30d67ad9
Bug 1302389 - patch 0 - Reftests for scroll-frame with vertical writing mode. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/90585022500bec26ef36b5ca22834b349ae01a53
Bug 1302389 - patch 1 - Rename ScrollFrameHelper::IsLTR to IsPhysicalLTR for clarity (no behavior change). r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/570b104585f4f3c4cfcaeee6f01bfd9704632942
Bug 1302389 - patch 2 - Add ScrollFrameHelper::IsBidiLTR to return the inline-bidi direction, as opposed to physical LTR-ness, and use this in GetScrolledFrameDir(). r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e502a75e1dd1ecda5126986ec456107d54b04bf
Bug 1302389 - patch 3 - Handle vertical writing mode in nsLayoutUtils::GetScrolledRect(). r=dholbert
Depends on: 1303916
You need to log in before you can comment on or make changes to this bug.