Closed Bug 1379334 Opened 4 years ago Closed 4 years ago

Make XULScrollFrame test for needing a vertical scrollbar because of the size of the horizontal scrollbar

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

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

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(2 files, 1 obsolete file)

Convert mis-indented code to 2-space indent, plus bracing and logical operator position fixes when reindenting
5.91 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
Make XULScrollFrame test for needing a vertical scrollbar because of the size of the horizontal scrollbar
6.18 KB, patch
dholbert
: review+
Details | Diff | Splinter Review 
This fixes an incremental layout bug, where the number of times we
reflow the frame affects its layout.  This is because we make the
decisions about the vertical scrollbar before the horizontal scrollbar
(and, when making the decision, adjust mHelper.mScrollPort for the size
of the scrollbar).  Thus, in order to avoid a situation where reflowing
the scrollframe once leads us to have no vertical scrollbar, but
reflowing it a second time leads us to add that scrollbar, we need to
retest for the need for a vertical scrollbar after we add the horizontal
one.

(It's possible there are some other missing cases here, but this is the
one that (a) already existed in the code and (b) is needed to fix the
reftest failure on Windows that I got on bug 1308876, in
layout/reftests/text-overflow/xulscroll.html .

The reftest here shows the bug even without bug 1308876 (though I
confirmed that only by loading the test and reference in a nightly
build, not in the reftest harness).  I did test that, in combination
with bug 1308876, the test fails without the patch and passes with the
patch.

MozReview-Commit-ID: LhMi7LbmB6J
This seems to be causing intermittent failures of layout/reftests/bugs/192767-*.xul reftests.
Attachment #8884471 - Flags: review?(dholbert)
This fixes an incremental layout bug, where the number of times we
reflow the frame affects its layout.  This is because we make the
decisions about the vertical scrollbar before the horizontal scrollbar
(and, when making the decision, adjust mHelper.mScrollPort for the size
of the scrollbar).  Thus, in order to avoid a situation where reflowing
the scrollframe once leads us to have no vertical scrollbar, but
reflowing it a second time leads us to add that scrollbar, we need to
retest for the need for a vertical scrollbar after we add the horizontal
one.

(It's possible there are some other missing cases here, but this is the
one that (a) already existed in the code and (b) is needed to fix the
reftest failure on Windows that I got on bug 1308876, in
layout/reftests/text-overflow/xulscroll.html .

The reftest here shows the bug even without bug 1308876 (though I
confirmed that only by loading the test and reference in a nightly
build, not in the reftest harness).  I did test that, in combination
with bug 1308876, the test fails without the patch and passes with the
patch.

MozReview-Commit-ID: LhMi7LbmB6J
Attachment #8885476 - Flags: review?(dholbert)
Attachment #8884471 - Attachment is obsolete: true
The new test is fuzzy on Android, and I'm inclined to just mark it as fuzzy-if(Android,54-54,8-8) and move on.
(By which I mean the scrollbar lengths are slightly different between test and reference, which I think is a sign of a separate Android scrolling bug.)
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #7)
> The new test is fuzzy on Android, and I'm inclined to just mark it as
> fuzzy-if(Android,54-54,8-8) and move on.

That sounds OK to me.
Comment on attachment 8885475 [details] [diff] [review]
Convert mis-indented code to 2-space indent, plus bracing and logical operator position fixes when reindenting

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

r=me

::: layout/generic/nsGfxScrollFrame.cpp
@@ +5160,5 @@
>          && styles.mHorizontal == NS_STYLE_OVERFLOW_AUTO) {
>  
>        if (!mHelper.mHasHorizontalScrollbar) {
> +          // no scrollbar?
> +        if (AddHorizontalScrollbar(aState, scrollbarBottom)) {

Looks like this "no scrollbar" code-comment is still over-indented by 2 spaces.
Attachment #8885475 - Flags: review?(dholbert) → review+
Comment on attachment 8885476 [details] [diff] [review]
Make XULScrollFrame test for needing a vertical scrollbar because of the size of the horizontal scrollbar

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

r=me
Attachment #8885476 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f676d48fd757855c0e0e52d0a3bde29c3ae4df2f
Bug 1379334 - Convert mis-indented code to 2-space indent, plus bracing and logical operator position fixes when reindenting.  r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/905f598f2051286c68f860cca497e75fb8cdeaa0
Bug 1379334 - Make XULScrollFrame test for needing a vertical scrollbar because of the size of the horizontal scrollbar.  r=dholbert
Pushed by dbaron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f676d48fd757
Convert mis-indented code to 2-space indent, plus bracing and logical operator position fixes when reindenting.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/905f598f2051
Make XULScrollFrame test for needing a vertical scrollbar because of the size of the horizontal scrollbar.  r=dholbert
https://hg.mozilla.org/mozilla-central/rev/f676d48fd757
https://hg.mozilla.org/mozilla-central/rev/905f598f2051
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Type: enhancement → defect
You need to log in before you can comment on or make changes to this bug.