855998 - Use Aero styling for hover and selected items in UI

Status: RESOLVED FIXED

(Toolkit :: Themes, enhancement)

Description

[u428464]

On Windows Vista and up the old styling for "list items" (library, sidebars) is still used, new one should be applied as seen for example on these mockups : http://people.mozilla.com/~shorlander/files/australis-design-specs/images/Australis-i01-DesignSpec-MainWindow-%28Sidebar-Expanded-Hover%29.jpg
http://people.mozilla.com/~shorlander/files/australis-design-specs/images/Australis-i01-DesignSpec-MainWindow-%28LibraryTab%29.jpg

Comment 1

[Richard Marti (:Paenglab)]

Attachment: proposed fix

This patch implements the treechildrens and listitems like they are already in TB with an addition from Optimizer to remove (or make it less visible) the double border, when multiple treechildren are selected. Where are also special rules to flatten the appearance under Windows 8.

I've also changed the sidebarheader appearance like in the mockup.

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 731453 [details] [diff] [review]
proposed fix

>+    background-image: linear-gradient(transparent, transparent);

What's the point of this?

>+/* Apply only on Windows 8 */
>+@media (-moz-windows-compositor) {
>+  @media not all and (-moz-windows-glass) {

This is a pretty ugly hack... And not quite correct, I think. Seems like you should also add -moz-windows-default-theme to the mix.

Comment 3

[Richard Marti (:Paenglab)]

(In reply to Dão Gottwald [:dao] from comment #2)
> Comment on attachment 731453 [details] [diff] [review]
> proposed fix
> 
> >+    background-image: linear-gradient(transparent, transparent);
> 
> What's the point of this?

This is a remnant from TB to overwrite the ...(odd) background-image. I'll remove it in the next patch.
 
> >+/* Apply only on Windows 8 */
> >+@media (-moz-windows-compositor) {
> >+  @media not all and (-moz-windows-glass) {
> 
> This is a pretty ugly hack... And not quite correct, I think. Seems like you
> should also add -moz-windows-default-theme to the mix.

Maybe a hack, but the only solution I know to apply only on Windows 8 in a CSS file. This works because -moz-windows-compositor applies on Vista/Win 7 and also on Windows 8 with the non-existent transparency and -moz-windows-glass applies only on Vista/Win7. Instead of -moz-windows-compositor I could use -moz-windows-default-theme.

Comment 4

[u428464]

(In reply to Richard Marti [:Paenglab] from comment #1)
> Created attachment 731453 [details] [diff] [review]
> proposed fix
> 
> This patch implements the treechildrens and listitems like they are already
> in TB with an addition from Optimizer to remove (or make it less visible)
> the double border, when multiple treechildren are selected. Where are also
> special rules to flatten the appearance under Windows 8.
> 
> I've also changed the sidebarheader appearance like in the mockup.

Nice ! Note that the mockup show a slightly lighter color that the TB current one for selected item.

Do you also change the search box in sidebars ?

Comment 5

[Richard Marti (:Paenglab)]

(In reply to Richard Marti [:Paenglab] from comment #3)
> Instead of -moz-windows-compositor I could use -moz-windows-default-theme.

Ah no, -moz-windows-compositor is needed. With -moz-windows-default-theme the rules are also applying on Win7 Basic.

(In reply to Guillaume C. [:ge3k0s] from comment #4)
> Nice ! Note that the mockup show a slightly lighter color that the TB
> current one for selected item.

The colors are the same as in Windows 7 Explorer.

> Do you also change the search box in sidebars ?

No, this should be done in a other bug. Actually also the sidebarheader change shouldn't be in this bug.

Comment 6

[u428464]

(In reply to Richard Marti [:Paenglab] from comment #5)
> (In reply to Guillaume C. [:ge3k0s] from comment #4)
> > Nice ! Note that the mockup show a slightly lighter color that the TB
> > current one for selected item.
> 
> The colors are the same as in Windows 7 Explorer.

You're right my eyes should have deceived me.

> > Do you also change the search box in sidebars ?
> 
> No, this should be done in a other bug. Actually also the sidebarheader change > shouldn't be in this bug.

Filed bug 856307 to track this.

Comment 7

[Frank Yan (:fryn)]

Attachment: screenshot of proposed fix on Windows 8

Comment 8

[Frank Yan (:fryn)]

Comment on attachment 731453 [details] [diff] [review]
proposed fix

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

I don't think I can provide very meaningful UI feedback on this one, as this mostly applies to Vista & 7, and I only have Windows 8 installations, both with HiDPI displays.
On Windows 8, the tree/list items don't quite look native, but they don't look bad either.

Technically:
There are a lot of repeated color values. Since CSS still doesn't have mixins and variables, perhaps we should make those %defines (like we do for many values in browser.css & browser-aero.css) for our preprocessor to handle to keep them in sync.

I think Dão would provide much better reviews for this bug.
Please address his feedback and flag him for the next iteration.

Comment 9

[Frank Yan (:fryn)]

(In reply to Richard Marti [:Paenglab] from comment #3)
> (In reply to Dão Gottwald [:dao] from comment #2)
> > >+/* Apply only on Windows 8 */
> > >+@media (-moz-windows-compositor) {
> > >+  @media not all and (-moz-windows-glass) {
> > 
> > This is a pretty ugly hack... And not quite correct, I think. Seems like you
> > should also add -moz-windows-default-theme to the mix.
> 
> Maybe a hack, but the only solution I know to apply only on Windows 8 in a
> CSS file. This works because -moz-windows-compositor applies on Vista/Win 7
> and also on Windows 8 with the non-existent transparency and
> -moz-windows-glass applies only on Vista/Win7. Instead of
> -moz-windows-compositor I could use -moz-windows-default-theme.

MattN might be of help here.

Comment 10

[u428464]

Does this bug also cover the fact to add a proper hover effect to items ?

Comment 11

[Richard Marti (:Paenglab)]

(In reply to Guillaume C. [:ge3k0s] from comment #10)
> Does this bug also cover the fact to add a proper hover effect to items ?

Yes

Comment 12

[Richard Marti (:Paenglab)]

Attachment: proposed fix v2

(In reply to Frank Yan (:fryn) from comment #8)
> Comment on attachment 731453 [details] [diff] [review]
> proposed fix
> 
> Review of attachment 731453 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> On Windows 8, the tree/list items don't quite look native, but they don't
> look bad either.

This is fixed in this patch.

> Technically:
> There are a lot of repeated color values. Since CSS still doesn't have
> mixins and variables, perhaps we should make those %defines (like we do for
> many values in browser.css & browser-aero.css) for our preprocessor to
> handle to keep them in sync.

I tried this but it doesn't replace the defines. This patch is with the colors on place. I'll add the patch with the placeholders in next attachment, so someone can check what is missing to work correctly.

Comment 13

[Richard Marti (:Paenglab)]

Attachment: patch v2 with color placeholders

Please can someone check what is missing or wrong? After compiling the placeholders are still in code.

Comment 14

[Frank Yan (:fryn)]

Comment on attachment 731962 [details] [diff] [review]
patch v2 with color placeholders

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

You need to update
/toolkit/themes/windows/global/jar.mn
for listbox-aero.css and prepend the listbox-aero line with an asterisk like we do for tree-aero.css, etc.

Comment 15

[Richard Marti (:Paenglab)]

(In reply to Frank Yan (:fryn) from comment #14)
> Comment on attachment 731962 [details] [diff] [review]
> patch v2 with color placeholders
> 
> Review of attachment 731962 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You need to update
> /toolkit/themes/windows/global/jar.mn
> for listbox-aero.css and prepend the listbox-aero line with an asterisk like
> we do for tree-aero.css, etc.

This is already done as it is using %include listbox.css. listbox.css is correctly included but the @...@ placeholders aren't exchanged by the values defined with %define. The same happens on tree-aero.css.

Comment 16

[Frank Yan (:fryn)]

Comment on attachment 731962 [details] [diff] [review]
patch v2 with color placeholders

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

Include the line:
%filter substitution
before the first %define.
https://developer.mozilla.org/en-US/docs/Build/Text_Preprocessor

Comment 17

[Richard Marti (:Paenglab)]

Attachment: patch v2 with color placeholders

(In reply to Frank Yan (:fryn) from comment #16)
> Comment on attachment 731962 [details] [diff] [review]
> patch v2 with color placeholders
> 
> Review of attachment 731962 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Include the line:
> %filter substitution
> before the first %define.
> https://developer.mozilla.org/en-US/docs/Build/Text_Preprocessor

Ah, yes this is the solution, thank you. This patch is working now. I'll let the other in the review. If the review ends with placeholders are needed, please check this patch if this would be okay like it is.

Comment 18

[u428464]

Review is needed here.

Comment 19

[u428464]

Still waiting on UI review and normal review.

Comment 20

[Stephen Horlander [:shorlander]]

Comment on attachment 731960 [details] [diff] [review]
proposed fix v2

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

Looks great thank you! Just noticed two things:

- Could we make the item height 21px instead of 20px to match Explorer and it would center the arrow?
- Is it possible to overlap the selected state by 1px in the list view? This would avoid the double border look when selecting more than one item.

Comment 21

[Richard Marti (:Paenglab)]

(In reply to Stephen Horlander from comment #20)
> Comment on attachment 731960 [details] [diff] [review]
> proposed fix v2
> 
> Review of attachment 731960 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great thank you! Just noticed two things:
> 
> - Could we make the item height 21px instead of 20px to match Explorer and
> it would center the arrow?

I tried to increase the min-height from 1.7em to 1.75em but it increased for 2px. Also with 1.705em it increased this 2px. I see no solution to make it 1px taller. Unfortunately calc doesn't work in the treechildren. Do somebody know a solution?

> - Is it possible to overlap the selected state by 1px in the list view? This
> would avoid the double border look when selecting more than one item.

The only solution I know is margin-top: -1px or margin-bottom: -1px but this cuts the border.

I leave the patch as it is for the r?. If someone has a solution, I'll implement it in a new patch.

Dão, if you more like the patch with placeholders, you can review this one. The rules are the same as the other patch.

Comment 22

[Justin Dolske [:Dolske]]

This doesn't really block Australis, it's just a theme refinement. (Still wanted, but I'd ship without it.)

Comment 23

[u428464]

BTW is there a specific reason that prevent review here ?

Comment 24

[Richard Marti (:Paenglab)]

Dão, this patch is awaiting review for more than two month. Please can you have a look on it?

Comment 25

[u428464]

If Dao has too many other patchs to review maybe someone else could look at this.

Comment 26

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 731960 [details] [diff] [review]
proposed fix v2

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

This looks good, but let's rebase on top of the patches in bug 810399 so that we don't need the media query hackz.

Apologies for the slow review turn-around-time.

::: toolkit/themes/windows/global/listbox-aero.css
@@ +11,5 @@
> +    -moz-margin-start: 1px;
> +    -moz-margin-end: 1px;
> +    border-width: 2px;
> +    border-radius: 3px;
> +    background-image: linear-gradient(transparent, transparent);

Can you add a comment about this?

@@ +52,5 @@
> +}
> +
> +/* Apply only on Windows 8 */
> +@media (-moz-windows-compositor) {
> +  @media not all and (-moz-windows-glass) {

Please rebase this off of the patches in bug 810399.

Comment 27

[Richard Marti (:Paenglab)]

Attachment: patch v3

Patch rebased on top of the patches in bug 810399.

(In reply to Jared Wein [:jaws] from comment #26)
> Comment on attachment 731960 [details] [diff] [review]
> proposed fix v2
> 
> Review of attachment 731960 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/themes/windows/global/listbox-aero.css
> @@ +11,5 @@
> > +    -moz-margin-start: 1px;
> > +    -moz-margin-end: 1px;
> > +    border-width: 2px;
> > +    border-radius: 3px;
> > +    background-image: linear-gradient(transparent, transparent);
> 
> Can you add a comment about this?

It's not needed here. I removed it.

> @@ +52,5 @@
> > +}
> > +
> > +/* Apply only on Windows 8 */
> > +@media (-moz-windows-compositor) {
> > +  @media not all and (-moz-windows-glass) {
> 
> Please rebase this off of the patches in bug 810399.

Changed to: @media (-moz-windows-version: win8)

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 767390 [details] [diff] [review]
patch v3

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

Cool, thanks! We'll just need to wait for the dependent bug to be fixed first.

::: toolkit/themes/windows/global/listbox-aero.css
@@ +59,5 @@
> +    outline: none;
> +  }
> +}
> +
> +/* Apply only on Windows 8 */

This comment is now redundant.

::: toolkit/themes/windows/global/tree-aero.css
@@ +163,5 @@
> +    color: -moz-DialogText;
> +  }
> +}
> +
> +/* Apply only on Windows 8 */

ditto

Comment 29

[Dão Gottwald [:dao]]

Comment on attachment 767390 [details] [diff] [review]
patch v3

>+    -moz-border-top-colors: @selectedBorderColor@ @whiteOpacityBorderColor@;
>+    -moz-border-right-colors: @selectedBorderColor@ @whiteOpacityBorderColor@;
>+    -moz-border-left-colors: @selectedBorderColor@ @whiteOpacityBorderColor@;
>+    -moz-border-bottom-colors: @selectedBorderColor@ @whiteOpacityBottomBorderColor@;

Can you make this a single border and an inset box-shadow (or two for the brighter bottom highlight)? This should simplify things.

>+/* Apply only on Windows 8 */
>+@media (-moz-windows-version: win8) {

You need to move this into the -moz-windows-default-theme block, as win8 doesn't imply that the default theme is being used.

Comment 30

[Richard Marti (:Paenglab)]

Attachment: patch v4

(In reply to Jared Wein [:jaws] from comment #28)
> Comment on attachment 767390 [details] [diff] [review]
> patch v3
> 
> Review of attachment 767390 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Cool, thanks! We'll just need to wait for the dependent bug to be fixed
> first.
> 
> ::: toolkit/themes/windows/global/listbox-aero.css
> @@ +59,5 @@
> > +    outline: none;
> > +  }
> > +}
> > +
> > +/* Apply only on Windows 8 */
> 
> This comment is now redundant.

Both comments removed.

(In reply to Dão Gottwald [:dao] from comment #29)
> Comment on attachment 767390 [details] [diff] [review]
> patch v3
> 
> >+    -moz-border-top-colors: @selectedBorderColor@ @whiteOpacityBorderColor@;
> >+    -moz-border-right-colors: @selectedBorderColor@ @whiteOpacityBorderColor@;
> >+    -moz-border-left-colors: @selectedBorderColor@ @whiteOpacityBorderColor@;
> >+    -moz-border-bottom-colors: @selectedBorderColor@ @whiteOpacityBottomBorderColor@;
> 
> Can you make this a single border and an inset box-shadow (or two for the
> brighter bottom highlight)? This should simplify things.

On tree this didn't work. In listboxes I've done it.

> >+/* Apply only on Windows 8 */
> >+@media (-moz-windows-version: win8) {
> 
> You need to move this into the -moz-windows-default-theme block, as win8
> doesn't imply that the default theme is being used.

Done.

Comment 31

[Dão Gottwald [:dao]]

Comment on attachment 768241 [details] [diff] [review]
patch v4

> > Can you make this a single border and an inset box-shadow (or two for the
> > brighter bottom highlight)? This should simplify things.
> 
> On tree this didn't work.

Too bad. Can you file a bug on this? Probably belongs in Core::Layout.

>+  #sidebar-header {
>+    -moz-appearance: none;
>+    background-color: #EEF3FA;
>+    border-bottom: none;
>+  }

Add text-shadow: none; color: black; here. Otherwise this looks miserable with lightweight themes.

Comment 32

[Richard Marti (:Paenglab)]

Attachment: patch for check-in

Patch addressing Dão's comment.

Comment 33

[Dão Gottwald [:dao]]

I guess this could actually land without bug 810399. The media query should fail gracefully and Windows 8 would use the Windows 7 / Vista style until bug 810399 lands.

Comment 34

[Richard Marti (:Paenglab)]

Okay, then I set checkin-needed after comment 33.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6637e9e5731

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [:dao] from comment #33)
> I guess this could actually land without bug 810399. The media query should
> fail gracefully and Windows 8 would use the Windows 7 / Vista style until
> bug 810399 lands.

I think MattN mentioned that the syntax for win8 detection was going to change, so the media query will need to be updated.

Comment 37

[Jim Mathies [:jimm]]

(In reply to Jared Wein [:jaws] from comment #36)
> (In reply to Dão Gottwald [:dao] from comment #33)
> > I guess this could actually land without bug 810399. The media query should
> > fail gracefully and Windows 8 would use the Windows 7 / Vista style until
> > bug 810399 lands.
> 
> I think MattN mentioned that the syntax for win8 detection was going to
> change, so the media query will need to be updated.

-moz-windows-version: win8 -> -moz-os-version: windows-win8

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Backed out for Win7/Win8 mochitest-other failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a36049f73557

https://tbpl.mozilla.org/php/getParsedLog.php?id=24787612&tree=Mozilla-Inbound

Comment 39

[Richard Marti (:Paenglab)]

Hmm, when I remove the "min-height: 1.7em" then the mochitests are passing. I'll try later if it works with 20px. But this would be suboptimal for taller font-size. I can also try to play with padding.

Or does somebody have an other soultion?

Comment 40

[Richard Marti (:Paenglab)]

Attachment: proposed fix

This patch runs all tests except one, see: https://tbpl.mozilla.org/php/getParsedLog.php?id=24839615&tree=Try

I don't know what I could do to solve this. Jared, do you know a solution? Or do you know who could help?

I changed in listbox-aero.css on listitem this:
- removed the min-height: 1.7em
- added padding-top and -bottom: 1px to have the correct height like on Explorer.

On tree-aero.css I changed on treechildren::-moz-tree-row the min-height: 1.7em to height: 1.8em.

On a previous try run I only removed both min-height and it passed all tests (https://tbpl.mozilla.org/?tree=Try&rev=5c65a9bf22e7)

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 770310 [details] [diff] [review]
proposed fix

I looked at the test and I'm not sure why it is failing. I went through the steps of the test manually and couldn't see any issue, but running the test locally did show the same failure.

Marco, as you wrote the test, do you have any idea why this is causing it to fail?

Comment 42

[Richard Marti (:Paenglab)]

It only doesn't fail when no padding is added to the listitem. I tried also to add the padding to the listcell instead of the listitem but it is still failing.

Comment 43

[Jared Wein [:jaws] (please needinfo? me)]

So is it showing a bug with the ensureElementIsVisible?

Comment 44

[Marco Bonardo [:mak]]

Sorry but I'm not the right person, I didn't write the test, maybe my name appears just as an old inbound sheriff.  You may want dbaron or masayuki if debugging it without deep layout knowledge is hard.

Comment 45

[Richard Marti (:Paenglab)]

Hi David and Masayuki, maybe you can help in this?

When I'm applying padding-top: 1px and padding-bottom: 1px at listitem, then one test is failing: 122 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/browser/components/places/tests/chrome/test_bug631374_tags_selector_scroll.xul | Scroll position is correct - got 1, expected 0 

Without the paddings the test passes. It also fails when applying a min-height: 1.7em. We need the same height to match the listitems (or tree items) in Explorer.

Jared is thinking in comment 43 it could be a bug in ensureElementIsVisible.

Thanks

Comment 46

[Richard Marti (:Paenglab)]

Please can somebody help solving this test failure?

Comment 47

[Jared Wein [:jaws] (please needinfo? me)]

Robert, maybe you can help Richard here?

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I can't tell easily what the bug is and I don't have time right now to debug it myself.

Try adding an alert() to the test when the test fails, so you can take a screenshot of the current test state when it fails. That should help you narrow down why the test failed.

Comment 49

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Marco Bonardo [:mak] from comment #44)
> Sorry but I'm not the right person, I didn't write the test, maybe my name
> appears just as an old inbound sheriff.  You may want dbaron or masayuki if
> debugging it without deep layout knowledge is hard.

No, you did write the test; see bug 631374 comment 11.

Comment 50

[Marco Bonardo [:mak]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #49)
> (In reply to Marco Bonardo [:mak] from comment #44)
> > Sorry but I'm not the right person, I didn't write the test, maybe my name
> > appears just as an old inbound sheriff.  You may want dbaron or masayuki if
> > debugging it without deep layout knowledge is hard.
> 
> No, you did write the test; see bug 631374 comment 11.

Sorry, but I looked at comment 38 and the failures were in
content/chrome/toolkit/content/tests/chrome/test_hiddenpaging.xul
content/chrome/toolkit/content/tests/chrome/test_mousescroll.xul
so I got confused.
Comment 45 is clearer, I can try to look at it.

Comment 51

[Marco Bonardo [:mak]]

just as a side note, in the Places code there is no special handling, we just use the methods exposed by the listbox to scroll to a given row:
getIndexOfFirstVisibleRow, getNumberOfVisibleRows, ensureIndexIsVisible

Since we don't play with the boxObject or pixel values, it's very likely the change is confusing the nsListBoxBodyFrame, maybe mRowHeight is not taking padding into account, the row height is set from GetFontMetricsForFrame()->MaxHeight() and I'm not sure what that represents. I will try to do some debugging later.
Dumb idea, maybe you could increase the row height using line-height instead of padding?

Comment 52

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [:mak] from comment #51)
> Dumb idea, maybe you could increase the row height using line-height instead
> of padding?

The line height should be determined by the font, hard-coding it doesn't seem like a great idea.

Comment 53

[Richard Marti (:Paenglab)]

(In reply to Marco Bonardo [:mak] from comment #51)
> Dumb idea, maybe you could increase the row height using line-height instead
> of padding?

I was curious if this could help, but the test is still failing. It still needs the padding. Only with line-height the listitem has not the needed height.

Comment 54

[Marco Bonardo [:mak]]

Attachment: relax test checks v1

I first verified if the listbox frame is taking the padding into account, and looks like it is properly considering it, according to my debugging.
I think we can relax a bit the test verification instead, we just care the tag that was previously visible is still visible, it may not be anymore the first visible tag though, there may be rounding and layout calculations that make that differ. We still just care it being visible, not the first one, that's an assumption we can't make safely.

Comment 55

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 775649 [details] [diff] [review]
relax test checks v1

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

::: browser/components/places/tests/chrome/test_bug631374_tags_selector_scroll.xul
@@ +127,5 @@
>              listItem.checked = false;
> +
> +            // Ensure the first visible tag is still visible in the list.
> +            let firstVisibleIndex = tagsSelector.getIndexOfFirstVisibleRow();
> +            let lastVisibleIndex = firstVisibleIndex + tagsSelector.getNumberOfVisibleRows() -1;

nit, space after '-'

@@ +130,5 @@
> +            let firstVisibleIndex = tagsSelector.getIndexOfFirstVisibleRow();
> +            let lastVisibleIndex = firstVisibleIndex + tagsSelector.getNumberOfVisibleRows() -1;
> +            let expectedTagIndex = tags.indexOf(firstVisibleTag);
> +            SimpleTest.ok(expectedTagIndex >= firstVisibleIndex &&
> +                          expectedTagIndex <= lastVisibleIndex,

I'm not sure how this would work, because in my manual testing the firstVisibleTag wouldn't be in the scrolled view, but if doing this loosens the restrictions enough to get the test passing I'm fine with it.

Comment 56

[Marco Bonardo [:mak]]

which manual testing did you execute? Steps?

Comment 57

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Marco Bonardo [:mak] from comment #56)
> which manual testing did you execute? Steps?

I had followed the steps of test_bug631374_tags_selector_scroll.xul.

Comment 58

[Richard Marti (:Paenglab)]

Comment on attachment 775649 [details] [diff] [review]
relax test checks v1

I made a try build to sure it doesn't fail (I had no doubts but I want to be sure): https://tbpl.mozilla.org/?tree=Try&rev=4790d65616ba

Comment 59

[Richard Marti (:Paenglab)]

Is it okay to set checkin-needed or is something missing?

Comment 60

[:Gijs (he/him)]

(In reply to Richard Marti [:Paenglab] from comment #59)
> Is it okay to set checkin-needed or is something missing?

You should add a new patch for checkin which also has the test fix, so it doesn't go orange once you check this in.

Comment 61

[Richard Marti (:Paenglab)]

Should I also add the patch from bug 888948 to be fixed in one push?

Comment 62

[Richard Marti (:Paenglab)]

Attachment: patch for check-in

Patch with test fix and fix from bug 888948

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7beb410b544

Comment 64

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c7beb410b544

Comment 65

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: screenshot.png

This patch broke non-places XUL trees pretty badly, see attached screenshot.

Is there a bug that tracks this regression?

Comment 66

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 785538 [details]
screenshot.png

Oh, the ghost-y row lines on the left I circled just because it seems wrong. Probably a separate problem.

The thing I care about is the circled stuff on the right.

Comment 67

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to ben turner [:bent] from comment #65)
> Is there a bug that tracks this regression?

Bug 898955 probably.