I'll get a screenshot when I'll be on my computer.

I was about to file a similar bug as well, for Windows 8 and 8.1. I think it would look best if the styling of the button would match the one of the refresh button.

(In reply to :Gijs Kruitbosch from bug 1131458 comment #9) > https://reviewboard.mozilla.org/r/3753/#review3009 > ::: browser/themes/windows/browser.css > (Diff revision 2) > > + padding: 0; > > Windows wants border: 0 none; as well, because otherwise you get ugly > win95-style 3d borders. I bet Linux is the same, but I'm rebuilding in my VM > right now so I won't know until half an hour. I'll update the bug (please > ping me if I forget) when I have a build. It looks like the patch was checked in without this. Margaret, do you have time to work on fixing this?

(In reply to :Gijs Kruitbosch from comment #3) > (In reply to :Gijs Kruitbosch from bug 1131458 comment #9) > > https://reviewboard.mozilla.org/r/3753/#review3009 > > > ::: browser/themes/windows/browser.css > > (Diff revision 2) > > > + padding: 0; > > > > Windows wants border: 0 none; as well, because otherwise you get ugly > > win95-style 3d borders. I bet Linux is the same, but I'm rebuilding in my VM > > right now so I won't know until half an hour. I'll update the bug (please > > ping me if I forget) when I have a build. > > It looks like the patch was checked in without this. Margaret, do you have > time to work on fixing this? No, sorry, I don't have time to work on this right now, since I've developed a bit of a backlog of Android bugs... hopefully someone from the desktop team can help me out here.

Comment on attachment 8568586 [details] [diff] [review] fix hover and active border on reader mode button in location bar, Review of attachment 8568586 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/windows/browser.css @@ +1580,5 @@ > > #reader-mode-button { > -moz-appearance: none; > padding: 0; > + border: 0 none; border-width: 0; is sufficient here. But then we are lacking a hover style on Windows and Linux (I agree that the pre-patch hover is quite ugly). (In reply to :Gijs Kruitbosch from bug 1131458 comment #9) > Windows wants border: 0 none; as well, because otherwise you get ugly > win95-style 3d borders. I bet Linux is the same This patch is missing a Linux change. Did you find that it wasn't needed?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6) > Comment on attachment 8568586 [details] [diff] [review] > fix hover and active border on reader mode button in location bar, > > Review of attachment 8568586 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/themes/windows/browser.css > @@ +1580,5 @@ > > > > #reader-mode-button { > > -moz-appearance: none; > > padding: 0; > > + border: 0 none; > > border-width: 0; is sufficient here. But then we are lacking a hover style > on Windows and Linux (I agree that the pre-patch hover is quite ugly). There is no hover style on OS X, and also no hover style for e.g. the page-report-button for popups, which is right next to it. Seems OK to me? > (In reply to :Gijs Kruitbosch from bug 1131458 comment #9) > > Windows wants border: 0 none; as well, because otherwise you get ugly > > win95-style 3d borders. I bet Linux is the same > > This patch is missing a Linux change. Did you find that it wasn't needed? See bug 1131458 comment 11: (In reply to :Gijs Kruitbosch from comment #11) > On Linux it seems you won't need to remove the border; I don't > see any for hover/active states.

(In reply to :Gijs Kruitbosch from comment #7) > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6) > > Comment on attachment 8568586 [details] [diff] [review] > > fix hover and active border on reader mode button in location bar, > > > > Review of attachment 8568586 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: browser/themes/windows/browser.css > > @@ +1580,5 @@ > > > > > > #reader-mode-button { > > > -moz-appearance: none; > > > padding: 0; > > > + border: 0 none; > > > > border-width: 0; is sufficient here. But then we are lacking a hover style > > on Windows and Linux (I agree that the pre-patch hover is quite ugly). > > There is no hover style on OS X, and also no hover style for e.g. the > page-report-button for popups, which is right next to it. Seems OK to me? ? This patch is changing behavior on Windows, where we have a hover style for the dropdown button, page-report-button, and the Refresh button. http://screencast.com/t/FelznILpe Without firing up my Linux build, I'm pretty sure that we show hover states for these on Linux, so we'll want to add one there.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8) > (In reply to :Gijs Kruitbosch from comment #7) > > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6) > > > Comment on attachment 8568586 [details] [diff] [review] > > > fix hover and active border on reader mode button in location bar, > > > > > > Review of attachment 8568586 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > ::: browser/themes/windows/browser.css > > > @@ +1580,5 @@ > > > > > > > > #reader-mode-button { > > > > -moz-appearance: none; > > > > padding: 0; > > > > + border: 0 none; > > > > > > border-width: 0; is sufficient here. But then we are lacking a hover style > > > on Windows and Linux (I agree that the pre-patch hover is quite ugly). > > > > There is no hover style on OS X, and also no hover style for e.g. the > > page-report-button for popups, which is right next to it. Seems OK to me? > > ? This patch is changing behavior on Windows, where we have a hover style > for the dropdown button, page-report-button, and the Refresh button. > http://screencast.com/t/FelznILpe We might have hover styles for the dropdown, page report and the refresh button on Windows, but we never did for the reader mode button until the ugly borders slipped in in bug 1131458. I don't think fixing a review nit from that bug should be held up while waiting for an asset to go add a hover effect on Windows/Linux.

Reader Mode Icon in SVG with three states.

So this works, but for some reason there's an extra focus rectangle drawn around the icon on OS X. I can't work out how to get rid of it (tried setting box-shadow to none on the toolbarbutton-icon which seemed not to help). Ideas? (I've also used a transform to move the icon down 1px, ideally shorlander should provide an icon where the path is fixed to be 1 lower than it is right now...)

Comment on attachment 8573259 [details] [diff] [review] use SVG and have a proper hover state for the reader mode button on Windows/Linux, Review of attachment 8573259 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/themes/osx/browser.css @@ +2545,1 @@ > #reader-mode-button:focus { This could be changed :-moz-focusring, so it only appears with keyboard navigation ::: browser/themes/shared/reader/reader-mode.svg @@ +1,3 @@ > +<?xml version="1.0" encoding="utf-8"?> > +<svg version="1.1" > + id="icon-readerMode" This id isn't needed @@ +6,5 @@ > + width="48" > + height="16"> > + <defs> > + <path id="glyphShape-readerMode-book" d="M5.5,4h-2C3.2,4,3,4.2,3,4.5C3,4.8,3.2,5,3.5,5h2 C5.8,5,6,4.8,6,4.5C6,4.2,5.8,4,5.5,4z M5.5,6h-2C3.2,6,3,6.2,3,6.5S3.2,7,3.5,7h2C5.8,7,6,6.8,6,6.5S5.8,6,5.5,6z M5.5,8h-2 C3.2,8,3,8.2,3,8.5C3,8.8,3.2,9,3.5,9h2C5.8,9,6,8.8,6,8.5C6,8.2,5.8,8,5.5,8z M15.4,1c0,0-3.1,0-4.4,0S8.1,1.5,8,3.3 C7.9,1.5,6.3,1,5,1S0.6,1,0.6,1C0.3,1,0,1.3,0,1.7v9.6C0,11.6,0.3,12,0.6,12c0,0,2.6,0,4.4,0c1.6,0,2.8,1,3,2.3 C8.2,13,9.4,12,11,12c1.8,0,4.4,0,4.4,0c0.4,0,0.6-0.4,0.6-0.8V1.7C16,1.3,15.7,1,15.4,1z M14,10L14,10c-0.2,0-1.6,0-3,0 c-1.6,0-2.9,0.8-3,2.2C7.9,10.8,6.6,10,5,10c-1.4,0-2.8,0-3,0v0c0,0,0,0,0,0V3c0,0,2.7,0,3.5,0C6.6,3,8,4.5,8,5.8 C8,4.5,9.4,3,10.5,3C11.3,3,14,3,14,3V10C14,10,14,10,14,10z" transform="translate(0, 1)"/> > + This blank line isn't needed (and the next ones too) @@ +9,5 @@ > + <path id="glyphShape-readerMode-book" d="M5.5,4h-2C3.2,4,3,4.2,3,4.5C3,4.8,3.2,5,3.5,5h2 C5.8,5,6,4.8,6,4.5C6,4.2,5.8,4,5.5,4z M5.5,6h-2C3.2,6,3,6.2,3,6.5S3.2,7,3.5,7h2C5.8,7,6,6.8,6,6.5S5.8,6,5.5,6z M5.5,8h-2 C3.2,8,3,8.2,3,8.5C3,8.8,3.2,9,3.5,9h2C5.8,9,6,8.8,6,8.5C6,8.2,5.8,8,5.5,8z M15.4,1c0,0-3.1,0-4.4,0S8.1,1.5,8,3.3 C7.9,1.5,6.3,1,5,1S0.6,1,0.6,1C0.3,1,0,1.3,0,1.7v9.6C0,11.6,0.3,12,0.6,12c0,0,2.6,0,4.4,0c1.6,0,2.8,1,3,2.3 C8.2,13,9.4,12,11,12c1.8,0,4.4,0,4.4,0c0.4,0,0.6-0.4,0.6-0.8V1.7C16,1.3,15.7,1,15.4,1z M14,10L14,10c-0.2,0-1.6,0-3,0 c-1.6,0-2.9,0.8-3,2.2C7.9,10.8,6.6,10,5,10c-1.4,0-2.8,0-3,0v0c0,0,0,0,0,0V3c0,0,2.7,0,3.5,0C6.6,3,8,4.5,8,5.8 C8,4.5,9.4,3,10.5,3C11.3,3,14,3,14,3V10C14,10,14,10,14,10z" transform="translate(0, 1)"/> > + > + <linearGradient id="gradient-state-default" x1="0%" y1 ="0%" x2 ="0" y2 ="100%"> > + <stop stop-color="#989898" offset = "0%"/> > + <stop stop-color="#808080" offset = "100%"/> It would be cleaner without spaces around "=". @@ +23,5 @@ > + <stop stop-color="#ff5500" offset = "100%"/> > + </linearGradient> > + > + <style type="text/css"> > + <![CDATA[ This line can be removed @@ +27,5 @@ > + <![CDATA[ > + .icon-state-default { fill: url(#gradient-state-default); } > + .icon-state-hover { fill: url(#gradient-state-hover); } > + .icon-state-pressed { fill: url(#gradient-state-pressed); } > + ]]> Same

This could be fixed by addressing bug 1140340 and reverting most of bug 1131458, although it looks like we'll still want to replace the PNG icon with an SVG.

(In reply to Dão Gottwald [:dao] from comment #14) > This could be fixed by addressing bug 1140340 and reverting most of bug > 1131458, although it looks like we'll still want to replace the PNG icon > with an SVG. What do you mean by "reverting most of bug 1131458", and how would that help? I don't think we should switch back to an image unless we have a more accessible alternative, and considering we're shipping on 38 and that's string-frozen, that seems like a non-starter.

(In reply to :Gijs Kruitbosch from comment #15) > (In reply to Dão Gottwald [:dao] from comment #14) > > This could be fixed by addressing bug 1140340 and reverting most of bug > > 1131458, although it looks like we'll still want to replace the PNG icon > > with an SVG. > > What do you mean by "reverting most of bug 1131458", and how would that help? I mean it should be an image, not a toolbar button. This would get rid of the border shown in attachment 8567844 [details]. That's what this bug seems to have been filed for. > I don't think we should switch back to an image unless we have a more > accessible alternative, and considering we're shipping on 38 and that's > string-frozen, that seems like a non-starter. The required strings seem to be there in readerMode.properties (readerMode.enter, readerMode.exit).

(In reply to Dão Gottwald [:dao] from comment #16) > (In reply to :Gijs Kruitbosch from comment #15) > > (In reply to Dão Gottwald [:dao] from comment #14) > > > This could be fixed by addressing bug 1140340 and reverting most of bug > > > 1131458, although it looks like we'll still want to replace the PNG icon > > > with an SVG. > > > > What do you mean by "reverting most of bug 1131458", and how would that help? > > I mean it should be an image, not a toolbar button. This would get rid of > the border shown in attachment 8567844 [details]. That's what this bug seems > to have been filed for. So would adding a single line of CSS, which was a missed review nit to begin with. Use the other bugs to do whatever you like, I think I'm done with the scope-creep here.

Deprioritized during backlog review. Removed from IT 39.2 priority backlog and not part of the initial Q2 campaign release.

Stephen, can you still attach the updated SVG, please?

This bug is most likely obsolete. We're gonna go with fixing bug 1140345 and that should probably take care of the issues described here.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22) > This bug is most likely obsolete. We're gonna go with fixing bug 1140345 and > that should probably take care of the issues described here. Can we resolve this, or is there anything left to fix here?

Verified fixed on Nightly 39.0a1 (2015-03-23), using Ubuntu 14.04 (x64), Windows 7 (x64) and Mac OS X 10.9.5.