Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: clarkbw, Assigned: bwinton)

Tracking

(Blocks 1 bug)

unspecified
mozilla40
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox38+ verified, firefox39 fixed, firefox40 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

In the mockup spec from bug 1145798 there is a footer area at the bottom of the reader view.

height: 64px 
background-color: #EBEBEB
shadow top separator:  (looks like #CDCDCD with 2px blur)

The footer spec also includes a button for removing the article from the reading list that will be filed as a separate bug.
Blocks: 1145813
Flags: qe-verify+
OS: Mac OS X → All
QA Contact: andrei.vaida
Hardware: x86 → All
(As a side note, it looks really quite strange in sepia and dark modes…  Do we maybe want some different styles for those?  Or perhaps modify the footer to match the toolbar?)
Teaser #2: https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/FooterSepia.png

(It looks like I'm taking this bug… ;)
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Points: --- → 3
Posted patch The first cut at the patch. (obsolete) — Splinter Review
Okay, how about this?  (There's a build for you to try out at https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/Transitions.dmg)
Attachment #8583889 - Flags: ui-review?(mmaslaney)
Blocks: 1132074
Iteration: --- → 39.3 - 30 Mar
Flags: firefox-backlog+
Duplicate of this bug: 1145813
Comment on attachment 8583889 [details] [diff] [review]
The first cut at the patch.

* New Sepia Background: #DEDAD4

* New Dark Background: #777

Button will remain the same color: #ebebeb with a gray stroke and slight drop shadow

The button type style should correlate with the selected type style for reading. For example, Serif button type, when Serif is selected.
Attachment #8583889 - Flags: ui-review?(mmaslaney) → ui-review+
Because we can't do inner shadows on only one side, I'm going to need mockups of how you want the top border to look for Sepia and Dark, too, so that I can steal their colours…  ;)
An honorable attempt to avoid comps :)

Mocks coming shortly...
The latest patch looks like this: https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/FooterThemes.png

(I realized I could use transparency, so we don't need comps!  ;)
Attachment #8583889 - Attachment is obsolete: true
Attachment #8584771 - Flags: ui-review+
Attachment #8584771 - Flags: review?(bmcbride)
I don't think "Delete this article" is the right text that we want for a couple reasons:
1) "Delete" is really jargon-y. It's a technical term and isn't related to the practice of a todo/reading list
2) The article isn't really "deleted" as it still exists online and could be revisited again

Could we instead go with:
"Remove from my reading list" or "Mark as read and remove"
Flags: needinfo?(matej)
I think "Remove from Reading List" would work well. Thanks.
Flags: needinfo?(matej)
You don't think that would be a little large for a button?
https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/FooterV2.png

(I've changed it in my tree, but wanted to double-check before I re-request review. :)
Flags: needinfo?(jaws)
Blake, the button appears to be a little dark.

Compare it with the spec: http://invis.io/4V2K8JN9E
Flags: needinfo?(bwinton)
Button

Default:

color: #FBFBFB / 251,251,251
border: #C1C1C1 / 193,193,193
shadow: 
 * x & y - 0px, 1px
 * blur - 2px
 * #000000 / 0,0,0 (5%)

Hover:

color: #EBEBEB / 251,251,251
border: #C1C1C1 / 193,193,193
shadow: 
 * x & y - 0px, 1px
 * blur - 2px
 * #000000 / 0,0,0 (5%)

Active:

color: #DADADA / 251,251,251
border: #C1C1C1 / 193,193,193
shadow: 
 * x & y - 0px, 1px
 * blur - 2px
 * #000000 / 0,0,0 (5%)
Posted patch The next version. (obsolete) — Splinter Review
I used 0,0,0 (10%) for the shadow colour, as per the spec.
Attachment #8584771 - Attachment is obsolete: true
Attachment #8584771 - Flags: review?(bmcbride)
Flags: needinfo?(jaws)
Flags: needinfo?(bwinton)
Attachment #8584908 - Flags: review?(bmcbride)
(In reply to Blake Winton (:bwinton) from comment #12)
> You don't think that would be a little large for a button?
> https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/FooterV2.png
> 

You're too large for a button.

I mean, yes, I see what you're saying. Maybe it can be "Remove from list" or even just "Remove," since the user is currently in the Reading List.
Comment on attachment 8584908 [details] [diff] [review]
The next version.

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

Hmm, the footer feels really awkward when using the dark theme. Gonna need to be inventive there.

::: toolkit/components/reader/AboutReader.jsm
@@ +57,5 @@
>  
>    this._setupStyleDropdown();
>    this._setupButton("close-button", this._onReaderClose.bind(this), "aboutReader.toolbar.close");
>    this._setupButton("share-button", this._onShare.bind(this), "aboutReader.toolbar.share");
> +  this._setupButton("remove-button", this._onReaderRemove.bind(this), "aboutReader.footer.removeThisArticle");

This needs to be gated behind the ReadingList pref check done below, and the entire footer should be hidden if the ReadingList is disabled.

@@ +59,5 @@
>    this._setupButton("close-button", this._onReaderClose.bind(this), "aboutReader.toolbar.close");
>    this._setupButton("share-button", this._onShare.bind(this), "aboutReader.toolbar.share");
> +  this._setupButton("remove-button", this._onReaderRemove.bind(this), "aboutReader.footer.removeThisArticle");
> +  let removeButton = this._doc.getElementById("remove-button");
> +  removeButton.textContent = gStrings.GetStringFromName("aboutReader.footer.removeThisArticle");

Nit: This seems better added as an option to _setupButton

@@ +278,5 @@
> +    if (!this._article || this._isReadingListItem == 0)
> +      return;
> +
> +    this._mm.sendAsyncMessage("Reader:RemoveFromList", { url: this._article.url });
> +    UITelemetry.addEvent("unsave.1", "button", null, "reader");

We probably want this UITelemetry event recorded such that we can tell it apart from the toolbar button, via the method param.

Also, nit: Duplicate code between this and _onReaderToggle.

::: toolkit/components/reader/content/aboutReader.html
@@ +25,5 @@
>      <div id="reader-message" class="message">
>      </div>
> +
> +    <div id="reader-footer" class="footer">
> +      <button id="remove-button" class="button remove-button font-size5"/>

The font-sizeX classes are intended only for the article text - doesn't feel safe to overload that. Better the font-size in the .remove-button class.

::: toolkit/locales/en-US/chrome/global/aboutReader.properties
@@ +25,5 @@
>  aboutReader.toolbar.openReadingList=Open Reading List
>  aboutReader.toolbar.closeReadingList=Close Reading List
>  aboutReader.toolbar.share=Share
>  
> +aboutReader.footer.removeThisArticle=Remove from Reading List

It's far far too late for string changes. I was able to land one recently only because it was new strings for hard blocker (ie, would have prevented us shipping), only after some discussion. This is comparatively really low priority change, so can almost guarantee this won't be allowed to land in 38.

::: toolkit/themes/windows/global/aboutReader.css
@@ +4,5 @@
>  
>  body {
>    padding: 64px 0;
>    max-width: 660px;
> +  margin: 0px auto;

Nit: We generally avoid specifying units when the value is 0.

@@ +248,5 @@
>    display: block;
>    background-position: center;
>    background-size: 24px 24px;
>    background-repeat: no-repeat;
> +  background-color: #FBFBFB;

Nit: Lowercase, like elsewhere in the file. However, see my comment below about refactoring this class.

@@ +413,5 @@
> +  height: 64px;
> +  background-color: #ebebeb;
> +  position: absolute;
> +  left: 41px;
> +  width: calc(100% - 41px);

Specifying 41px here makes me cringe. But conveniently, it's not needed:
  left: 0;
  width: 100%;

@@ +426,5 @@
> +    rgba(205, 205, 205, .555)
> +    rgba(205, 205, 205, .444)
> +    rgba(205, 205, 205, .333)
> +    rgba(205, 205, 205, .222)
> +    rgba(205, 205, 205, .111);

This is.... something. :)

I'd far prefer one of the alternatives:
* Use a linear-gradient
* Use an inset box shadow. If you specify a negative spread-radius, you can make it appear only on one edge. eg:
  box-shadow: 0px 9px 9px -9px #c1c1c1 inset;

IMO, bow-shadow is the better option. It looks more natural than a linear gradient.

@@ +433,5 @@
> +.sepia .footer {
> +  background-color: #dedad4;
> +}
> +
> +.remove-button {

This class would be much simpler if this button didn't use the .button class. Or, even better, the .button class was refactored so that the stuff that only applies to the toolbar was under a ".toolbar .button" selector.

@@ +452,5 @@
> +  font-family: "Fira Sans", Helvetica, Arial, sans-serif;
> +}
> +
> +.serif .remove-button {
> +  font-family: "Charis SIL", Georgia, "Times New Roman", serif;

Don't duplicate this - add the selector to the existing rule for .serif/.sans-serif
Attachment #8584908 - Flags: review?(bmcbride) → review-
(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #17)
> Hmm, the footer feels really awkward when using the dark theme. Gonna need
> to be inventive there.

So, I switched it to the #777 that Michael recommended above, let's see how he feels about that…

(There's a Mac build for testing at https://dl.dropboxusercontent.com/u/2301433/Firefox/Reading/Transitions.dmg )

> ::: toolkit/components/reader/AboutReader.jsm
> @@ +57,5 @@
> >  
> >    this._setupStyleDropdown();
> >    this._setupButton("close-button", this._onReaderClose.bind(this), "aboutReader.toolbar.close");
> >    this._setupButton("share-button", this._onShare.bind(this), "aboutReader.toolbar.share");
> > +  this._setupButton("remove-button", this._onReaderRemove.bind(this), "aboutReader.footer.removeThisArticle");
> 
> This needs to be gated behind the ReadingList pref check done below, and the
> entire footer should be hidden if the ReadingList is disabled.

Done.

> @@ +59,5 @@
> >    this._setupButton("close-button", this._onReaderClose.bind(this), "aboutReader.toolbar.close");
> >    this._setupButton("share-button", this._onShare.bind(this), "aboutReader.toolbar.share");
> > +  this._setupButton("remove-button", this._onReaderRemove.bind(this), "aboutReader.footer.removeThisArticle");
> > +  let removeButton = this._doc.getElementById("remove-button");
> > +  removeButton.textContent = gStrings.GetStringFromName("aboutReader.footer.removeThisArticle");
> Nit: This seems better added as an option to _setupButton

Added.  (I figured since we were only doing it once, it didn't make a lot of sense to add, but conceptually it belongs there, so I put it in.)

> @@ +278,5 @@
> > +    UITelemetry.addEvent("unsave.1", "button", null, "reader");
> We probably want this UITelemetry event recorded such that we can tell it
> apart from the toolbar button, via the method param.
> 
> Also, nit: Duplicate code between this and _onReaderToggle.

Fixed, and fixed!  (I hope.  Let me know if there's an alternate way you'ld like me to do this.)

> ::: toolkit/components/reader/content/aboutReader.html
> @@ +25,5 @@
> > +      <button id="remove-button" class="button remove-button font-size5"/>
> The font-sizeX classes are intended only for the article text - doesn't feel
> safe to overload that. Better the font-size in the .remove-button class.

Done.

> ::: toolkit/locales/en-US/chrome/global/aboutReader.properties
> @@ +25,5 @@
> > +aboutReader.footer.removeThisArticle=Remove from Reading List
> It's far far too late for string changes. I was able to land one recently
> only because it was new strings for hard blocker (ie, would have prevented
> us shipping), only after some discussion. This is comparatively really low
> priority change, so can almost guarantee this won't be allowed to land in 38.

Okay, I'm using the existing string instead, and will file a followup bug.

> ::: toolkit/themes/windows/global/aboutReader.css
> @@ +4,5 @@
> > +  margin: 0px auto;
> Nit: We generally avoid specifying units when the value is 0.

Fixed everywhere in that file.

> @@ +248,5 @@
> > +  background-color: #FBFBFB;
> Nit: Lowercase, like elsewhere in the file. However, see my comment below
> about refactoring this class.

Fixed everywhere in that file.

> @@ +413,5 @@
> > +  left: 41px;
> > +  width: calc(100% - 41px);
> Specifying 41px here makes me cringe. But conveniently, it's not needed:
>   left: 0;
>   width: 100%;

Fixed!  (Thanks!  I don't know why I didn't do that before…)

> @@ +426,5 @@
> > +    rgba(205, 205, 205, .555)
> > +    rgba(205, 205, 205, .444)
> > +    rgba(205, 205, 205, .333)
> > +    rgba(205, 205, 205, .222)
> > +    rgba(205, 205, 205, .111);
> 
> This is.... something. :)

Heh.  You should have seen the previous version, before I figured out I could use transparency…  ;)

> I'd far prefer one of the alternatives:
> * Use a linear-gradient
> * Use an inset box shadow. If you specify a negative spread-radius, you can
> make it appear only on one edge. eg:
>   box-shadow: 0px 9px 9px -9px #c1c1c1 inset;
> 
> IMO, box-shadow is the better option. It looks more natural than a linear
> gradient.

Yes, and box-shadow was in the spec, so that's what I used.  Thank you for letting me know how to do the single-edge box-shadow!

> @@ +433,5 @@
> > +.sepia .footer {
> > +  background-color: #dedad4;
> > +}
> > +
> > +.remove-button {
> This class would be much simpler if this button didn't use the .button
> class. Or, even better, the .button class was refactored so that the stuff
> that only applies to the toolbar was under a ".toolbar .button" selector.

I refactored it, but it didn't seem to save me as much as I hoped…  Still, it's more refactored now, so that's something, right?  :)

> @@ +452,5 @@
> > +  font-family: "Fira Sans", Helvetica, Arial, sans-serif;
> > +}
> > +
> > +.serif .remove-button {
> > +  font-family: "Charis SIL", Georgia, "Times New Roman", serif;
> 
> Don't duplicate this - add the selector to the existing rule for
> .serif/.sans-serif

Changed!

Thanks,
Blake.
Attachment #8584908 - Attachment is obsolete: true
Attachment #8585590 - Flags: ui-review?(mmaslaney)
Attachment #8585590 - Flags: review?(bmcbride)
Attachment #8585590 - Flags: ui-review?(mmaslaney) → ui-review+
Comment on attachment 8585590 [details] [diff] [review]
Once more, with feeling!  :)

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

\o/
Attachment #8585590 - Flags: review?(bmcbride) → review+
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e0854136b830
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla40
[Tracking Requested - why for this release]: Because the rest of reading mode has landed in 38.

Uplift request:

[User impact if declined]: The footer will be missing from the reader mode.
[Describe test coverage new/current, TBPL]: Manual testing, and baking on mozilla-central for two days.
[Risks and why]: Low-ish risk because its a fair-sized change but it only affects reading mode.
[String/UUID change made/needed]: none
Comment on attachment 8585590 [details] [diff] [review]
Once more, with feeling!  :)

(I think bwinton meant to flip these flags)
Attachment #8585590 - Flags: approval-mozilla-beta?
Attachment #8585590 - Flags: approval-mozilla-aurora?
Comment on attachment 8585590 [details] [diff] [review]
Once more, with feeling!  :)

should be in 38 beta 2
Attachment #8585590 - Flags: approval-mozilla-beta?
Attachment #8585590 - Flags: approval-mozilla-beta+
Attachment #8585590 - Flags: approval-mozilla-aurora?
Attachment #8585590 - Flags: approval-mozilla-aurora+
Depends on: 1151793
Depends on: 1151795
Verified fixed on Beta 38.0b2-build1 (20150406174117), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Two bugs found and filed separately, one affecting the button's hover styling on high-contrast themes (Bug 1151795) and the other affecting the footer's UI in case of narrow browser windows (Bug 1151793).
Status: RESOLVED → VERIFIED
No longer depends on: 1154063
Removing qe-verify flag, as verification on 38 Beta should suffice here.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.