Use new close icons for Linux GTK theme

VERIFIED FIXED in Firefox 29

Status

()

defect
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: mconley, Assigned: MattN)

Tracking

(Blocks 1 bug, {addon-compat})

unspecified
Firefox 31
All
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox29 verified, firefox30 verified, firefox31 verified)

Details

(Whiteboard: [Australis:P4+])

Attachments

(3 attachments, 8 obsolete attachments)

3.20 KB, image/svg+xml
Details
2.73 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
12.99 KB, patch
MattN
: review+
Details | Diff | Splinter Review
We switched Windows and OSX to new close icons for Australis in bug 851001, but didn't yet for the Linux GTK theme because we don't have icons for them yet.

We should update the close icons for the Linux GTK theme as well, if we can.
Assignee: nobody → mnoorenberghe+bmo
Status: NEW → ASSIGNED
Hardware: x86_64 → All
Whiteboard: [Australis:M?] → [Australis:M7]
This patch only handles the close button on the tabs and removes the -moz-appearance: button styling (which gives a button border on hover) in order to match the live demo at http://people.mozilla.com/~shorlander/files/australis-linux-svg-test/australis-liveDemo-linux.html

Stephen, I some questions:
1) which of the following should use the new icon
2) which of those should also have the moz-appearance: button style removed?
3) which of the following should use the hover the active states?

A) tabstrip close button (browser.tabs.closeButtons = 3)
B) Find bar
C) Notification bars
D) Popup Notifications (aka. doorhanger notifications)
E) web notification floating panel - example: Notification.requestPermission(); 
                                              new Notification("foo");
F) Menu items (e.g. File > Close on the view source window or the Close Tab context menu) when enabled in the Gnome appearance settings. (see gnome-tweak-tool > Theme > Menus Have Icons)
G) Dialog buttons with icons. (see gnome-tweak-tool > Theme > Buttons Have Icons)
H) New tab button undo notification close button
I) Add-on bar

Note that there were two different sizes of moz-icon://stock/gtk-close: size=menu (16x16) and size=button (22x22). Can I scale the svg icon to 22x22?
Attachment #763171 - Flags: feedback?(shorlander)
Comment on attachment 763171 [details] [diff] [review]
WIP 1 - Just the tab close button

>+.tabbrowser-tab:not([selected]) > .tab-stack > .tab-content > .tab-close-button:not(:active):not(:hover) {

.tab-close-button:not([selected]):not(:active):not(:hover) {
(In reply to Matthew N. [:MattN] from comment #2)

> G) Dialog buttons with icons. (see gnome-tweak-tool > Theme > Buttons Have
> Icons)

All of those things should be consistent except this.


> Note that there were two different sizes of moz-icon://stock/gtk-close:
> size=menu (16x16) and size=button (22x22). Can I scale the svg icon to 22x22?

It could maybe scale to that size… Where would we use the 22 x 22 size?
(Reporter)

Comment 5

6 years ago
Removing the items from M7 that do not block us from landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P4]
Duplicate of this bug: 900842
Given that Australis is now on Aurora, do you still plan to replace the close button? Currently the "close" and "newtab" icons are seem to be the only GTK stock/theme icons in use and it looks kind of out of place this way. As for "close": even native GTK apps like Gedit and Nautilus do not use this icon anymore.
(Reporter)

Updated

5 years ago
Flags: needinfo?(MattN+bmo)
(In reply to Michael Monreal [:monreal] from comment #7)
> Given that Australis is now on Aurora, do you still plan to replace the
> close button?

Yes. There are many Australis bugs which will get fixed on Aurora.
Flags: needinfo?(MattN+bmo)
Whiteboard: [Australis:M?][Australis:P4] → [Australis:P4+]
I updated MattN's patch to cover all the close buttons and with dao's comment.

Would be great if we could get this in since they look very out of place now that we updated the rest of the toolbar icons.
Flags: needinfo?(MattN+bmo)
Stephen, you're missing quite a few: https://mxr.mozilla.org/mozilla-central/search?string=gtk-close

Will you have time to do the rest or shall I?
Flags: needinfo?(MattN+bmo)
Extensions will probably want to update their close icons to use the "close-icon" class.
Keywords: addon-compat
Comment on attachment 763171 [details] [diff] [review]
WIP 1 - Just the tab close button

I'm working on finishing this now.
Attachment #763171 - Attachment is obsolete: true
What should we do about light LWT. The screenshot shows this would make them very hard to see and I don't think we can uplift without fixing that. Can I use use the normal state for background tabs with a light LWT or do you want to tweak the image or add a new state for it?
Flags: needinfo?(shorlander)
(In reply to Matthew N. [:MattN] from comment #13)
> Created attachment 8391780 [details]
> New close icons are hard to see on a light LWT
> 
> What should we do about light LWT. The screenshot shows this would make them
> very hard to see and I don't think we can uplift without fixing that. Can I
> use use the normal state for background tabs with a light LWT or do you want
> to tweak the image or add a new state for it?

We could use the normal state, but there is no guarantee that it will pick up a color that works with light LWTs. I guess we need a hardcoded black and white close icon for LWTs.
Flags: needinfo?(shorlander)
OK, I guess I'll wait for those images before final review.

I want to note in this bug that the reason the patch uses background-image instead of list-style-image is because apparently -moz-image-region doesn't work on SVG files.

This makes it harder to do comment 2 point F so I propose we leave the menu item images as-is for now. I also think the GTK icons look better than the new SVG in this case and it'll be consistent with G which Stephen said we should leave using GTK icons in comment 4.

Stephen, are you fine with leaving comment 2 part F alone?

(In reply to Stephen Horlander [:shorlander] from comment #4)
> > Note that there were two different sizes of moz-icon://stock/gtk-close:
> > size=menu (16x16) and size=button (22x22). Can I scale the svg icon to 22x22?
> 
> It could maybe scale to that size… Where would we use the 22 x 22 size?

It was only for point G which you said to leave alone so we should be fine with 16x16.
Flags: needinfo?(shorlander)
Want to do a pre-review while shorlander figures out new icons for LWT?
Attachment #8389571 - Attachment is obsolete: true
Attachment #8391803 - Flags: feedback?(gijskruitbosch+bugs)

Comment 17

5 years ago
Comment on attachment 8391803 [details] [diff] [review]
WIP 2 - Everything except the LWT contrast issue

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

This mostly looks sane, although I'm not 100% sure why you needed to adjust margins/paddings in so many places. Are those just orthogonal fixes?

Other than that, I believe this could do with a try run to check that it doesn't badly regress perf, because svg. :-(

::: browser/themes/linux/tabview/tabview.css
@@ +91,5 @@
>    width: 16px;
>    height: 16px;
>    opacity: 0.2;
> +  background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 25%, 100%, 0);
> +  background-position: center center;

Why do you need this? The height and width is the same as some of the other elements you're touching, so wouldn't it just fill up all of the rect anyway?
Attachment #8391803 - Flags: feedback?(gijskruitbosch+bugs) → feedback+

Comment 18

5 years ago
FWIW, if we want to contain risk just landing the changes for the tab close button might be a reasonable compromise.
(In reply to :Gijs Kruitbosch from comment #17)
> This mostly looks sane, although I'm not 100% sure why you needed to adjust
> margins/paddings in so many places. Are those just orthogonal fixes?

Stephen added the margins IIRC and I believe it's because removing the -moz-appearance removed some spacing around the button (and the border on :hover). Adding padding makes the clickable area not so small. In some cases adding padding didn't work and I don't know why and didn't want to fuss with it at this point so I used margins.

> Other than that, I believe this could do with a try run to check that it
> doesn't badly regress perf, because svg. :-(

Yep, I almost forgot:
Baseline: https://tbpl.mozilla.org/?tree=Try&rev=b06d6a826be8
SVG Close: https://tbpl.mozilla.org/?tree=Try&rev=3c6b75d8dfcc

> ::: browser/themes/linux/tabview/tabview.css
> @@ +91,5 @@
> >    width: 16px;
> >    height: 16px;
> >    opacity: 0.2;
> > +  background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 25%, 100%, 0);
> > +  background-position: center center;
> 
> Why do you need this? The height and width is the same as some of the other
> elements you're touching, so wouldn't it just fill up all of the rect anyway?

This is an HTML stylesheet that doesn't inherit the other styles. Yes it is 16px but I figured I'd use the same styles here as the XUL one.
(In reply to Matthew N. [:MattN] from comment #19)
> (In reply to :Gijs Kruitbosch from comment #17)
> > Other than that, I believe this could do with a try run to check that it
> > doesn't badly regress perf, because svg. :-(
> 
> Yep, I almost forgot:
> Baseline: https://tbpl.mozilla.org/?tree=Try&rev=b06d6a826be8
> SVG Close: https://tbpl.mozilla.org/?tree=Try&rev=3c6b75d8dfcc

Comparison: http://compare-talos.mattn.ca/?oldRevs=b06d6a826be8&newRev=3c6b75d8dfcc&submit=true

Comment 21

5 years ago
(In reply to Matthew N. [:MattN] from comment #20)
> (In reply to Matthew N. [:MattN] from comment #19)
> > (In reply to :Gijs Kruitbosch from comment #17)
> > > Other than that, I believe this could do with a try run to check that it
> > > doesn't badly regress perf, because svg. :-(
> > 
> > Yep, I almost forgot:
> > Baseline: https://tbpl.mozilla.org/?tree=Try&rev=b06d6a826be8
> > SVG Close: https://tbpl.mozilla.org/?tree=Try&rev=3c6b75d8dfcc
> 
> Comparison:
> http://compare-talos.mattn.ca/
> ?oldRevs=b06d6a826be8&newRev=3c6b75d8dfcc&submit=true

I retriggered this some more. It seems fine to me. Do we still want this for 29? If so, we should get a move on...
(In reply to :Gijs Kruitbosch from comment #21)
> I retriggered this some more. It seems fine to me. Do we still want this for
> 29? If so, we should get a move on...

Yeah, that's my fault. Will try and get to it today.
Flags: needinfo?(shorlander)
Cleaned up the SVG a lot, added icons for LWT Themes.
Flags: needinfo?(MattN+bmo)
Attachment #761489 - Attachment is obsolete: true
Flags: needinfo?(MattN+bmo)
Attachment #8396689 - Attachment description: close.svg → Linux Close Icon - 02
Thanks Stephen.
Attachment #8391780 - Attachment is obsolete: true
Attachment #8391803 - Attachment is obsolete: true
Attachment #8396994 - Flags: review?(gijskruitbosch+bugs)

Comment 25

5 years ago
Comment on attachment 8396994 [details] [diff] [review]
v.1 SVG with 6 image states (2 LWT)

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

This generally looks good, but there's a couple of bits that aren't quite right yet.

::: browser/base/content/newtab/newTab.xul
@@ +30,5 @@
>            <xul:button id="newtab-undo-restore-button" tabindex="-1"
>                        label="&newtab.undo.restoreButton;"
>                        class="newtab-undo-button" />
>            <xul:toolbarbutton id="newtab-undo-close-button" tabindex="-1"
> +                             class="close-icon tabbable"

Might make sense to split this + the associated CSS change up into a separate patch to make blame clearer, but don't feel obliged if that seems like make-work.

::: browser/themes/linux/browser.css
@@ +1673,4 @@
>  .customization-tipPanel-closeBox > .close-icon {
> +  -moz-appearance: none;
> +  width: 16px;
> +  height: 16px;

The :active state here is the same as the non-hover state.

::: browser/themes/linux/tabview/tabview.css
@@ +100,5 @@
> +  background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 33.333%, 100%, 16.667%);
> +}
> +
> +.close:hover:active {
> +  background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 50%, 100%, 33.333%);

I'm not seeing the :active state being any different from the :hover state in the tabview.

::: toolkit/themes/linux/global/alerts/alert.css
@@ +61,5 @@
>    color: -moz-activehyperlinktext;
>  }
>  
>  .alertCloseButton {
> +  -moz-appearance: none;

I wonder vaguely if this is working, because the icon seems very squeezed into the corner...

::: toolkit/themes/linux/global/global.css
@@ +309,5 @@
> +
> +/* :::::: Close button icons ::::: */
> +
> +.close-icon {
> +  background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 16.667%, 100%, 0);

Some part of me would feel better about these percentages if they were calc(100% / 6) or something, but I guess that's not any more or less rational.

(couldn't resist, sorry. But in all seriousness, it's probably OK to leave like this, especially if we're only using this for 16x16 icons so the rounding errors are unlikely to matter at all)

::: toolkit/themes/linux/global/icons/close.svg
@@ +9,5 @@
> +     y="0px"
> +     width="96px"
> +     height="16px"
> +     viewBox="0 0 96 16"
> +     xml:space="preserve">

Out of curiosity, why is this necessary?
Attachment #8396994 - Flags: review?(gijskruitbosch+bugs) → feedback+
(In reply to :Gijs Kruitbosch from comment #25)
> ::: browser/base/content/newtab/newTab.xul
> @@ +30,5 @@
> >            <xul:button id="newtab-undo-restore-button" tabindex="-1"
> >                        label="&newtab.undo.restoreButton;"
> >                        class="newtab-undo-button" />
> >            <xul:toolbarbutton id="newtab-undo-close-button" tabindex="-1"
> > +                             class="close-icon tabbable"
> 
> Might make sense to split this + the associated CSS change up into a
> separate patch to make blame clearer, but don't feel obliged if that seems
> like make-work.
Attachment #8397640 - Flags: review?(gijskruitbosch+bugs)
Attachment #8397640 - Attachment is obsolete: true
Attachment #8397640 - Flags: review?(gijskruitbosch+bugs)
Attachment #8397644 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch (no internet 29 Mar - 6 Apr) from comment #25)
> Comment on attachment 8396994 [details] [diff] [review]
> v.1 SVG with 6 image states (2 LWT)
> 
> ::: browser/themes/linux/browser.css
> @@ +1673,4 @@
> >  .customization-tipPanel-closeBox > .close-icon {
> > +  -moz-appearance: none;
> > +  width: 16px;
> > +  height: 16px;
> 
> The :active state here is the same as the non-hover state.

Hmm… I couldn't figure out why this doesn't work (the prior button style didn't have an active state either). I think it's some platform bug related to panels and I thought I got it working after deleting most of the panel attribute (not @id) but then I couldn't get it to work again. Another weird thing is that locking the hover and active states in DOMi works fine and I don't see any styles that would donflict. Would you rather I leave it as-is with the new default and hover states or should I revert to the gtk icon? I don't want to block this bug on that issue as its quite weird.

> ::: browser/themes/linux/tabview/tabview.css
> @@ +100,5 @@
> > +  background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 33.333%, 100%, 16.667%);
> > +}
> > +
> > +.close:hover:active {
> > +  background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 50%, 100%, 33.333%);
> 
> I'm not seeing the :active state being any different from the :hover state
> in the tabview.

There was no hover/active state before this patch either. Shall I leave the new styles in place so its ready for another bug to fix or revert to GTK?
 
> ::: toolkit/themes/linux/global/alerts/alert.css
> @@ +61,5 @@
> >    color: -moz-activehyperlinktext;
> >  }
> >  
> >  .alertCloseButton {
> > +  -moz-appearance: none;
> 
> I wonder vaguely if this is working, because the icon seems very squeezed
> into the corner...

I copied the padding from Windows now. If the -moz-appearance wasn't working there would be a border.

> ::: toolkit/themes/linux/global/global.css
> @@ +309,5 @@
> > +
> > +/* :::::: Close button icons ::::: */
> > +
> > +.close-icon {
> > +  background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 16.667%, 100%, 0);
> 
> Some part of me would feel better about these percentages if they were
> calc(100% / 6) or something, but I guess that's not any more or less
> rational.
> 
> (couldn't resist, sorry. But in all seriousness, it's probably OK to leave
> like this, especially if we're only using this for 16x16 icons so the
> rounding errors are unlikely to matter at all)

This is a fine idea and I considered as I was doing the math. I decided not to since calc can lead to things not being cached and I'd rather this SVG-as-image be cached if possible. I'm not sure if it actually matters in this case but I also don't think this will be changing often.

> ::: toolkit/themes/linux/global/icons/close.svg
> @@ +9,5 @@
> > +     y="0px"
> > +     width="96px"
> > +     height="16px"
> > +     viewBox="0 0 96 16"
> > +     xml:space="preserve">
> 
> Out of curiosity, why is this necessary?

Which part specifically? the xml:space? You may want to ask Stephen instead since he made this.
Attachment #8396994 - Attachment is obsolete: true
Attachment #8397658 - Flags: review?(gijskruitbosch+bugs)

Updated

5 years ago
Attachment #8397644 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 29

5 years ago
(In reply to Matthew N. [:MattN] from comment #28)
> Created attachment 8397658 [details] [diff] [review]
> v.2 SVG with 6 image states (2 LWT)
> 
> (In reply to :Gijs Kruitbosch (no internet 29 Mar - 6 Apr) from comment #25)
> > Comment on attachment 8396994 [details] [diff] [review]
> > v.1 SVG with 6 image states (2 LWT)
> > 
> > ::: browser/themes/linux/browser.css
> > @@ +1673,4 @@
> > >  .customization-tipPanel-closeBox > .close-icon {
> > > +  -moz-appearance: none;
> > > +  width: 16px;
> > > +  height: 16px;
> > 
> > The :active state here is the same as the non-hover state.
> 
> Hmm… I couldn't figure out why this doesn't work (the prior button style
> didn't have an active state either). I think it's some platform bug related
> to panels and I thought I got it working after deleting most of the panel
> attribute (not @id) but then I couldn't get it to work again. Another weird
> thing is that locking the hover and active states in DOMi works fine and I
> don't see any styles that would donflict. Would you rather I leave it as-is
> with the new default and hover states or should I revert to the gtk icon? I
> don't want to block this bug on that issue as its quite weird.

Huh, weird. Going to spend a bit of time on it, but I think we can followup if I also can't figure this out (which is likely enough).

> 
> > ::: browser/themes/linux/tabview/tabview.css
> > @@ +100,5 @@
> > > +  background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 33.333%, 100%, 16.667%);
> > > +}
> > > +
> > > +.close:hover:active {
> > > +  background-image: -moz-image-rect(url("chrome://global/skin/icons/close.svg"), 0, 50%, 100%, 33.333%);
> > 
> > I'm not seeing the :active state being any different from the :hover state
> > in the tabview.
> 
> There was no hover/active state before this patch either. Shall I leave the
> new styles in place so its ready for another bug to fix or revert to GTK?

Follow-up fodder, in that case. Sorry, I should have checked the pre-patch state as well.

> > ::: toolkit/themes/linux/global/icons/close.svg
> > @@ +9,5 @@
> > > +     y="0px"
> > > +     width="96px"
> > > +     height="16px"
> > > +     viewBox="0 0 96 16"
> > > +     xml:space="preserve">
> > 
> > Out of curiosity, why is this necessary?
> 
> Which part specifically? the xml:space? You may want to ask Stephen instead
> since he made this.

Yes, the xml:space. I'm pretty sure it doesn't matter, none of the SVG's content seems space-dependent? I'll try it while I poke at the rest of the new patches.
It's not strictly necessary. Could just use:

<svg version="1.1" 
     id="icon-close" 
     xmlns="http://www.w3.org/2000/svg" 
     xmlns:xlink="http://www.w3.org/1999/xlink"
     width="96px" 
     height="16px">

Comment 31

5 years ago
Comment on attachment 8397658 [details] [diff] [review]
v.2 SVG with 6 image states (2 LWT)

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

r+ assuming the padding was all you changed (bugzilla's interdiff is broken because I made you split up the patches, sigh).

Please file a followup bug about panorama. The issue there is that it's <div>'s so they don't get an :active pseudoclass unless you make them focusable, but they are divs and only have a click handler so making them focusable would be a lie because you couldn't focus them and then hit space/enter to activate them. It should really just be a <toolbarbutton> with -moz-appearance: none, but let's not hold this issue up for that.

Adding CSS for the customize tips close button in browser.css (same selector as the one where you're setting width/height/moz-appearance) for .close-icon:active without :hover to show the 'active' icon fixes that problem (although of course it means that if you click down and then 'drag' out, it keeps :active style, which isn't quite right either...). Up to you whether you think that's better than the current state.
Attachment #8397658 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 32

5 years ago
(In reply to Stephen Horlander [:shorlander] from comment #30)
> It's not strictly necessary. Could just use:
> 
> <svg version="1.1" 
>      id="icon-close" 
>      xmlns="http://www.w3.org/2000/svg" 
>      xmlns:xlink="http://www.w3.org/1999/xlink"
>      width="96px" 
>      height="16px">


Hrm. But actually, thinking about this... why are we using percentages at all? Because the SVG itself is sized in px, so all the -moz-image-regions/rects could also be. With the bonus that if we had to add stuff to the SVG that'd change its size, we wouldn't need to update all the things. Matt, is there something I'm missing?
Flags: needinfo?(MattN+bmo)
(In reply to Stephen Horlander [:shorlander] from comment #30)
> It's not strictly necessary. Could just use:…
OK, I removed @xml:space.

(In reply to :Gijs Kruitbosch from comment #31)
> Please file a followup bug about panorama. The issue there is that it's
> <div>'s so they don't get an :active pseudoclass unless you make them
> focusable, but they are divs and only have a click handler so making them
> focusable would be a lie because you couldn't focus them and then hit
> space/enter to activate them. It should really just be a <toolbarbutton>
> with -moz-appearance: none, but let's not hold this issue up for that.

OK, filed bug 989122 (not blocking this since it wasn't caused by this patch).

> Adding CSS for the customize tips close button in browser.css (same selector
> as the one where you're setting width/height/moz-appearance) for
> .close-icon:active without :hover to show the 'active' icon fixes that
> problem (although of course it means that if you click down and then 'drag'
> out, it keeps :active style, which isn't quite right either...). Up to you
> whether you think that's better than the current state.

OK, I added the :active rule with a comment.

(In reply to :Gijs Kruitbosch (no internet 29 Mar - 6 Apr) from comment #32)
> Hrm. But actually, thinking about this... why are we using percentages at
> all? Because the SVG itself is sized in px, so all the
> -moz-image-regions/rects could also be. With the bonus that if we had to add
> stuff to the SVG that'd change its size, we wouldn't need to update all the
> things. Matt, is there something I'm missing?

No, you're right that it's more maintainable. Initially I thought it would be handled differently but it seems to scale fine.
Attachment #8397658 - Attachment is obsolete: true
Attachment #8398189 - Flags: review+
Flags: needinfo?(MattN+bmo)
Comment on attachment 8398189 [details] [diff] [review]
v.3 SVG with 6 image states with :active on the customization tip r=Gijs

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A. This is a design change.
User impact if declined: Users will continue to see ugly GTK close icons on Linux. Linux users are regularly asking me when we'll stop using them.
Testing completed (on m-c, etc.): locally by myself and Gijs
Risk to taking this patch (and alternatives if risky): Low risk IMO since many of the uses of the close icon are non-essential (other than tabs, tab groups, and the find bar). Work case would be minor size/layout issues around the close icons or the icon not showing at all.
String or IDL/UUID changes made by this patch: None
Attachment #8398189 - Flags: approval-mozilla-beta?
Attachment #8398189 - Flags: approval-mozilla-aurora?
Comment on attachment 8397644 [details] [diff] [review]
Use the "tabbable" class to make the new tab page's close button tabbable

[Approval Request Comment]
See comment 35. This is some cleanup that the other patch builds on top of.
Attachment #8397644 - Flags: approval-mozilla-beta?
Attachment #8397644 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/dc1fa1b8f9ee
https://hg.mozilla.org/mozilla-central/rev/a80b1a1a9700
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Attachment #8397644 - Flags: approval-mozilla-beta?
Attachment #8397644 - Flags: approval-mozilla-beta+
Attachment #8397644 - Flags: approval-mozilla-aurora?
Attachment #8397644 - Flags: approval-mozilla-aurora+
Attachment #8398189 - Flags: approval-mozilla-beta?
Attachment #8398189 - Flags: approval-mozilla-beta+
Attachment #8398189 - Flags: approval-mozilla-aurora?
Attachment #8398189 - Flags: approval-mozilla-aurora+
Keywords: verifyme
Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux i686; rv:30.0) Gecko/20100101 Firefox/30.0
Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0

Verified that the new close icons are used in Firefox 29 beta 4 (20140331125246), Aurora 30.0a2 (20140401004001) and Nightly 31.0a1 (20140331030201) under Ubuntu 32-bit.
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: petruta.rasa
You need to log in before you can comment on or make changes to this bug.