1149261 - Improve the Reader View Close Button

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Close_Buttons.zip

We need to replace the close icon, in addition to some right and bottom-border CSS coloring (see spec).

http://invis.io/J42KOU2N5

Comment 1

[Blake Winton (:bwinton) (:☕️)]

I could take this…

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Attachment: The first cut at the patch.

Before I send this over to Unfocused for review, did we want to transition the colours on hover, or is it okay this way?

Build (without hover transitions) up at https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/Transitions.dmg

Thanks,
Blake.

Comment 3

[Michael Maslaney [:mmaslaney] ([e-mail])]

Comment on attachment 8585831 [details] [diff] [review]
The first cut at the patch.

Any chance we can have the left-border to adopt red, as well?

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Sadly, no, because it comes from the sidebar.  But it's not there when the sidebar is collapsed…  ;)

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8585831 [details] [diff] [review]
The first cut at the patch.

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

Didn't get through all this (I'm just taking some time off, so unblocking by submitting my comments now), but the SVG at least needs work.

::: toolkit/themes/shared/reader/RM-Close-24x24.svg
@@ +1,2 @@
>  <?xml version="1.0" encoding="utf-8"?>
> +<!-- Generator: Adobe Illustrator 18.1.1, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->

Ugh, wow do I hate what Illustrator does to SVGs... lets tidy this up a bit (at least this file isn't nearly as bad as some I've seen).

Remove this comment. Replace with MPL license header.

@@ +3,2 @@
>  <svg version="1.1" id="Icons" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
> +	 viewBox="0 0 24 24" enable-background="new 0 0 24 24" xml:space="preserve">

Remove: enable-background and xml:space attributes, they do nothing

@@ +3,3 @@
>  <svg version="1.1" id="Icons" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
> +	 viewBox="0 0 24 24" enable-background="new 0 0 24 24" xml:space="preserve">
> +<g enable-background="new    ">

Remove this entire <g> block and everything in it (yea, seriously).

@@ +39,3 @@
>  <g>
> +	<polygon fill="#808080" points="20.477,6.551 20.477,17.449 11.992,17.449 11.992,20 23,20 23,4 11.992,4 11.992,6.551 	"/>
> +	<polygon fill="#808080" points="1,11.981 9.698,19.95 9.698,15.13 18.184,15.13 18.184,8.87 9.698,8.87 9.698,4.011 	"/>

Fix up the trailing whitespace before the final " in these two lines.

::: toolkit/themes/shared/reader/RM-Close-hover-24x24.svg
@@ +1,2 @@
>  <?xml version="1.0" encoding="utf-8"?>
> +<!-- Generator: Adobe Illustrator 18.1.1, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->

Ditto here, though since the shape is the same, the ideal would be to do something like what browser/themes/shared/readinglist/icons.svg does, using CSS in the SVG.

Comment 6

[:Margaret Leibovic]

FYI, jaws or myself can probably help with these reviews.

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Attachment: The next version of the patch, with much better SVG.

Wow, the code is _way_ smaller and cleaner now!  :)

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Screenshots at https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/CloseButtonV2.png

Comment 9

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

Comment on attachment 8587432 [details] [diff] [review]
The next version of the patch, with much better SVG.

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

Can you file a bug about having the Reader Mode toolbar use proper RTL styling? Right now it is only designed to be shown on the left side (note that the bug will also need to transform:scaleX(-1); the icons in the toolbar).

::: toolkit/components/reader/content/aboutReader.html
@@ +49,5 @@
>        </li>
>      </ul>
>      <li><button id="toggle-button" class="button toggle-button" hidden="true"/></li>
>      <li><button id="list-button" class="button list-button" hidden="true"/></li>
> +    <li class="spacer"/>

We don't need this if we tweak how we apply borders to the buttons in the toolbar.

::: toolkit/themes/windows/global/aboutReader.css
@@ -253,5 @@
>    padding: 0;
>    list-style: none;
>    background-color: #fbfbfb;
>    -moz-user-select: none;
> -  border-right: 1px solid #b5b5b5;

I think we should leave this here, and then add margin-right:-1px to `.toolbar .button`

@@ +498,1 @@
>    background-color: #d94141;

We should be showing a darker red color for :active. The other buttons get darker on :active, also it would be better to use :hover:active.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #9)
> Can you file a bug about having the Reader Mode toolbar use proper RTL
> styling? Right now it is only designed to be shown on the left side (note
> that the bug will also need to transform:scaleX(-1); the icons in the
> toolbar).

Definitely.  Bug 1150759 has been filed.

> ::: toolkit/components/reader/content/aboutReader.html
> @@ +49,5 @@
> -  border-right: 1px solid #b5b5b5;
> I think we should leave this here, and then add margin-right:-1px to
> `.toolbar .button`

Hmmm…  Okay, I'll give that a try.

> @@ +498,1 @@
> >    background-color: #d94141;
> 
> We should be showing a darker red color for :active. The other buttons get
> darker on :active, also it would be better to use :hover:active.

Michael, which colour red should we use for active?

Comment 11

[Michael Maslaney [:mmaslaney] ([e-mail])]

AE2325

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8587432 [details] [diff] [review]
The next version of the patch, with much better SVG.

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

::: toolkit/themes/shared/reader/RM-Close-24x24.svg
@@ +1,2 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

The license header should be placed before the <?xml /> tag

@@ +1,5 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg version="1.1" id="Icons"

You can get rid of the id and the version, they don't have an impact on how icons look.

@@ +4,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg version="1.1" id="Icons"
> +  xmlns="http://www.w3.org/2000/svg"
> +  xmlns:xlink="http://www.w3.org/1999/xlink"
> +  x="0px" y="0px" viewBox="0 0 24 24">

The x and y attributes have no effect on the root svg element.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Attachment: The next version of the patch.

(In reply to Tim Nguyen [:ntim] from comment #13)
> ::: toolkit/themes/shared/reader/RM-Close-24x24.svg
> @@ +1,2 @@
> > +<?xml version="1.0" encoding="utf-8"?>
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> The license header should be placed before the <?xml /> tag

Nope.  http://www.w3.org/TR/2006/REC-xml11-20060816/#NT-prolog says that the XMLDecl must come before anything else, including comments.
(Also, I tested it, and it breaks if you put the comment first.  ;)

I did implement the rest of your suggestions, though!  :)

And this should contain the fixed jaws asked for, too.  I did need to leave the border-right on the .toolbar .button rule, otherwise the button's backgrounds would hide the border.

Comment 15

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

Comment on attachment 8588615 [details] [diff] [review]
The next version of the patch.

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

::: toolkit/themes/windows/global/aboutReader.css
@@ +510,5 @@
> +  border-bottom: 1px solid #d94141;
> +  border-right: 1px solid #d94141;
> +}
> +
> +.close-button:active{

space before curly bracket, and plz use :hover:active

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Attachment: The final version, ready for checkin.

Approval Request Comment
[Feature/regressing bug #]: readinglist
[User impact if declined]: We will have a lot of buttons that look like Xs, which is confusing to the user.
[Describe test coverage new/current, TreeHerder]: Manual testing.
[Risks and why]: Reasonably low risk, due to lack of code changes.  (Only css and images.)
[String/UUID change made/needed]: None.

Comment 17

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/integration/fx-team/rev/1fbced09450f

Comment 18

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/1fbced09450f

Comment 19

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/302de53f3fb9

Comment 20

[(PTO June 19-July 7) Ryan VanderMeulen]

Backed out for Windows bustage. Please rebase for Aurora uplift.
https://hg.mozilla.org/releases/mozilla-aurora/rev/b361047e79ec

https://treeherder.mozilla.org/logviewer.html#?job_id=751131&repo=mozilla-aurora

Comment 21

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

Blake, it looks like the aero reference needs to be removed from the toolkit/themes/windows/global/jar.mn in order to get this uplifted to aurora and beta, since Dao's work from bug 1150867 is only on mozilla-central.

Comment 22

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

Pushed to aurora with that line removed,
https://hg.mozilla.org/releases/mozilla-aurora/rev/fcece72d4512

Comment 23

[Blake Winton (:bwinton) (:☕️)]

So, assuming https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=fcece72d4512 is fine, there's nothing I need to do?  (Thanks, Jared!)

Comment 24

[(PTO June 19-July 7) Ryan VanderMeulen]

Yeah, we're good here. Thanks :)

Comment 25

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-beta/rev/a3c18ef98317

Comment 26

[Andrei Vaida [:avaida]]

Verified fixed on Beta 38.0b4-build1 (20150413143743) using Ubuntu 14.04 (x64), Windows 7 (x64) and Mac OS X 10.9.5.

Comment 27

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Removing qe-verify flag as verification on Firefox 38 Beta should suffice.