1136716 - Discrepancy between the tooltip displayed for the two Reader Mode close buttons

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Andrei Vaida [:avaida]]

Reproducible on:
Nightly 39.0a1 (2015-02-25), Aurora 38.0a2 (2015-02-25)

Affected platforms:
Windows 7 (x64), Ubuntu 14.04 (x64), Mac OS X 10.9.5

Steps to reproduce:
1. Launch Firefox with a clean profile.
2. Enable Reader Mode by setting 'reader.parse-on-load.enabled' to true. Restart to apply the pref change.
3. Open a website that can be displayed in Reader Mode - (e.g.) http://www.bbc.com/sport/0/football/31620713
4. Click the Reader Mode button from the Location Bar.
5. Hover the mouse cursor over the Reader Mode button placed on the Location Bar. Check the tooltip.
6. Hover the mouse cursor over the Reader Mode close button placed on the Reader Mode sidebar. Check the tooltip.

Expected result:
Both of the buttons display the same tooltip stating the exit action they will trigger on click.

Actual result:
The button placed on the Location Bar states: "Exit Reader View", while the button placed on the sidebar states: "Close Reader View".

Comment 1

[:Margaret Leibovic]

This was actually implemented in bug 1131303.

This inconsistency may be acceptable, since one button in inside the reader view, and the other is outside.

mmaslaney, what do you think we should do here?

Comment 2

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

I agree with you Margaret, that this inconsistency may be acceptable, because the triggers are in different locations. If we had to choose one, I would suggest "Close Reader View" as it better relates to the "x" icon.

Comment 3

[:Margaret Leibovic]

clarbw, can you make a call here? I'm happy to write a patch if we need to change this, but we should do it soon if so, since it involves a string change.

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to :Margaret Leibovic from comment #3)
> clarbw, can you make a call here? I'm happy to write a patch if we need to
> change this, but we should do it soon if so, since it involves a string
> change.

We have a 2 week auto-pass to uplift non-string changes so I'll take that to mean we can uplift a string change this week without too much fuss.  If you feel like you can make the change tomorrow (Friday) lets do it. Otherwise I think we can just own this until later.

Comment 5

[:Margaret Leibovic]

Okay, I'll whip up a patch to just use "Close Reader View" everywhere for consistency.

Comment 6

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1136716/margaret

/r/4431 - Bug 1136716 - Consolidate reader view strings in one .properties file, and make the strings to close reader view consistent. r=bnicholson,Unfocused

Pull down this commit:

hg pull review -r e50c9c99a919a660fb064fc452c0cee62a04ee43

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8570636 [details]
MozReview Request: bz://1136716/margaret

https://reviewboard.mozilla.org/r/4429/#review3651

Ship It!

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 8570636 [details]
MozReview Request: bz://1136716/margaret

https://reviewboard.mozilla.org/r/4429/#review3693

Ship It!

Comment 9

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/26c42369d83a

Comment 10

[:Margaret Leibovic]

Comment on attachment 8570636 [details]
MozReview Request: bz://1136716/margaret

Approval Request Comment
[Feature/regressing bug #]: reading list/reader view
[User impact if declined]: tooltips/a11y text will be inconsistent between urlbar button and reader view controls
[Describe test coverage new/current, TreeHerder]: tested locally, just landed on m-c
[Risks and why]: low-risk string change
[String/UUID change made/needed]: this patch removes 4 entities, and adds 2, but in reality it just consolidates existing strings, so it should hopefully be easy to localize

I'm assuming this is okay because of the reader view/reading list string freeze exemption, but it's also not the end of the world if this just rides the trains.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26c42369d83a

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8570636 [details]
MozReview Request: bz://1136716/margaret

We can take this since it's early and because of the reading list exemption.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/191e9c8fcfad

Comment 14

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 39.0a1 (2015-03-04) and Aurora 38.0a2 (2014-03-05), using Ubuntu 14.04 (x64), Mac OS X 10.9.5 and Windows 7 (x64). 

Both tooltips now state: 'Close Reader View'.

Comment 16

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1136716 - Consolidate reader view strings in one .properties file, and make the strings to close reader view consistent. r=bnicholson,Unfocused