Closed Bug 1149261 Opened 5 years ago Closed 5 years ago

Improve the Reader View Close Button

Categories

(Firefox :: General, defect, P1)

38 Branch
x86
All
defect
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 40
Iteration:
40.1 - 13 Apr
Tracking Status
firefox38 --- verified
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: mmaslaney, Assigned: bwinton)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 3 obsolete files)

Attached file 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
I could take this…
Assignee: nobody → bwinton
Attached patch The first cut at the patch. (obsolete) — Splinter Review
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.
Attachment #8585831 - Flags: ui-review?(mmaslaney)
Status: NEW → ASSIGNED
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?
Sadly, no, because it comes from the sidebar.  But it's not there when the sidebar is collapsed…  ;)
Attachment #8585831 - Flags: ui-review?(mmaslaney) → ui-review+
Attachment #8585831 - Flags: review?(bmcbride)
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.
Attachment #8585831 - Flags: review?(bmcbride) → review-
FYI, jaws or myself can probably help with these reviews.
Wow, the code is _way_ smaller and cleaner now!  :)
Attachment #8585831 - Attachment is obsolete: true
Attachment #8587432 - Flags: ui-review?(mmaslaney)
Attachment #8587432 - Flags: review?(jaws)
Iteration: --- → 40.1 - 13 Apr
Points: --- → 2
Attachment #8587432 - Flags: ui-review?(mmaslaney) → ui-review+
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.
Attachment #8587432 - Flags: review?(jaws) → feedback+
Blocks: 1150759
(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?
Flags: needinfo?(mmaslaney)
AE2325
Flags: needinfo?(mmaslaney)
Duplicate of this bug: 1147587
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.
Attached patch The next version of the patch. (obsolete) — Splinter Review
(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.
Attachment #8587432 - Attachment is obsolete: true
Attachment #8588615 - Flags: ui-review+
Attachment #8588615 - Flags: review?(jaws)
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
Attachment #8588615 - Flags: review?(jaws) → review+
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.
Attachment #8588615 - Attachment is obsolete: true
Attachment #8588652 - Flags: ui-review+
Attachment #8588652 - Flags: review+
Attachment #8588652 - Flags: approval-mozilla-beta?
Attachment #8588652 - Flags: approval-mozilla-aurora?
Keywords: checkin-needed
Blocks: 1132074
Flags: qe-verify?
Flags: firefox-backlog+
Target Milestone: Firefox 38 → ---
https://hg.mozilla.org/mozilla-central/rev/1fbced09450f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
Attachment #8588652 - Flags: approval-mozilla-beta?
Attachment #8588652 - Flags: approval-mozilla-beta+
Attachment #8588652 - Flags: approval-mozilla-aurora?
Attachment #8588652 - Flags: approval-mozilla-aurora+
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.
So, assuming https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=fcece72d4512 is fine, there's nothing I need to do?  (Thanks, Jared!)
Flags: needinfo?(bwinton)
Yeah, we're good here. Thanks :)
Verified fixed on Beta 38.0b4-build1 (20150413143743) using Ubuntu 14.04 (x64), Windows 7 (x64) and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
Flags: qe-verify? → qe-verify+
QA Contact: andrei.vaida
Removing qe-verify flag as verification on Firefox 38 Beta should suffice.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.