Implement infopanel to promote Reader View when first available

VERIFIED FIXED in Firefox 38.0.5

Status

()

Firefox
Tours
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Dolske, Assigned: MattN)

Tracking

(Depends on: 4 bugs, Blocks: 1 bug)

unspecified
Firefox 40
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox37 unaffected, firefox38 unaffected, firefox38.0.5 verified, firefox39 fixed, firefox40 verified)

Details

(Whiteboard: [reader-ui])

Attachments

(3 attachments, 16 obsolete attachments)

8.03 KB, image/png
Details
3.23 KB, image/png
Details
26.78 KB, patch
MattN
: review+
MattN
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
Created attachment 8566349 [details]
Current UX mock

This bug covers implementing two infopanels to help promote the Reading View button and Reading List sidebar the first time they're seen.

1) The first time the Reading List sidebar is shown, display a "Welcome to Reading List" (all strings TBD) panel, with a "Show me" button to open a tour page (including bug 1134497), URL TBD.

Since we only want to show this once, this should set a pref to prevent showing it again.

2) The first time a user visits a page that can be viewed in Reader View, display an infopanel pointing out that this button is available in inviting them to click it. (This will open the page in Reader View, which should also show the Reading List sidebar the first time, which will in turn show the infopanel from #1 inviting them to take a tour of the feature).

As with #1, we want a pref to make sure this is only shown once.


For both infopanels, we want to suppress (blacklist) their display on www.mozilla.org where another UITour might be active -- this avoids these panels dueling with panels from past/future UITours. EG, if you're on a tour page that has opened the Hello panel, it would be really confusing to also have one of this bug's infopanels open at the same time (or to cause the Hello panel to close).


One edge case: If the user is on a blacklisted page, and clicks the Reader View button (without having seen the infopanel), we should ideally set the pref to avoid showing the infopanel that promotes the button (since they've clearly already discovered it themselves). But, just to be clear, if they're on a readable-but-blacklisted page and they do _not_ click the button, we'd still want to show the infopanel on the first non-blacklisted page (since they clearly haven't discovered the button!).
(Reporter)

Comment 1

2 years ago
Matt: did you want to have this blacklist talk to UITour to see if the page is currently using UITour, or just globally blacklist all the pages that are able to (potentially) access UITour? The latter might be simpler, and since we'd just be deferring the infopanel until the user visits a non-mozilla.org page, having an overly broad blacklist isn't a big deal.
Flags: needinfo?(MattN+bmo)
(Reporter)

Updated

2 years ago
Blocks: 1132951
Required for Campaign Release
Blocks: 1132074
Flags: qe-verify?
Flags: firefox-backlog+
(In reply to Justin Dolske [:Dolske] from comment #1)
> Matt: did you want to have this blacklist talk to UITour to see if the page
> is currently using UITour, or just globally blacklist all the pages that are
> able to (potentially) access UITour? The latter might be simpler, and since
> we'd just be deferring the infopanel until the user visits a non-mozilla.org
> page, having an overly broad blacklist isn't a big deal.

The latter is probably safer, easier and less fragile. I say safer since we only know a page is using UITour once they call one of the APIs and it's possible that both the page and reader view code race on DOMContentLoaded.

The code that's attempting to show the info panels for the first time can check the permission manager to see if UITour is allowed (and the page is on HTTPS since UITour isn't allowed on non-HTTPS unless browser.uitour.requireSecure is false). If it's allowed to use UITour, don't show the info panel.
Flags: needinfo?(MattN+bmo)

Updated

2 years ago
Points: --- → 5
(Reporter)

Updated

2 years ago
Priority: -- → P1
The most up to date wireframe for the tour will be found in the nonobsolete attachment in bug 1129536.
Flags: qe-verify? → qe-verify+
QA Contact: andrei.vaida
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 40.1 - 13 Apr
Michael, can you provide the image that should be in the info panel for "1. Via detected article" of this spec, https://bug1129536.bugzilla.mozilla.org/attachment.cgi?id=8574210 ?
Flags: needinfo?(mmaslaney)
Created attachment 8586775 [details] [diff] [review]
Patch for Reader View infopanel

This is without the associated graphic, but I guess we could land that in a follow-up if we don't get it sooner.
Attachment #8586775 - Flags: review?(bmcbride)
Created attachment 8586906 [details] [diff] [review]
Patch

Patch for panels when Reader View button becomes available the first time and when Reading List is opened the first time
Attachment #8586775 - Attachment is obsolete: true
Attachment #8586775 - Flags: review?(bmcbride)
Attachment #8586906 - Flags: review?(bmcbride)
Created attachment 8586909 [details] [diff] [review]
Patch v1.1
Attachment #8586906 - Attachment is obsolete: true
Attachment #8586906 - Flags: review?(bmcbride)
Attachment #8586909 - Flags: review?(bmcbride)
Attachment #8586909 - Flags: review?(bmcbride) → review?(gijskruitbosch+bugs)
Jared, I believe Holly created that graphic.
Flags: needinfo?(mmaslaney)
Holly, can you provide the graphic mentioned in comment #5?
Flags: needinfo?(hhabstritt.bugzilla)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> Holly, can you provide the graphic mentioned in comment #5?

Sure, Jared. What are the dimension requirements for that area? I'm not sure if my mock-ups are to scale with how the door hangers are built.
Flags: needinfo?(hhabstritt.bugzilla) → needinfo?(jaws)
(In reply to Holly Habstritt Gaal [:Habber] from comment #11)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> > Holly, can you provide the graphic mentioned in comment #5?
> 
> Sure, Jared. What are the dimension requirements for that area? I'm not sure
> if my mock-ups are to scale with how the door hangers are built.

Can you provide them in 48x48 and 96x96px?
Flags: needinfo?(jaws) → needinfo?(hhabstritt.bugzilla)
Whiteboard: [reader-ui]
Created attachment 8588899 [details]
Readerview_doorhanger_FTE@2x (96x96)
Flags: needinfo?(hhabstritt.bugzilla)
Created attachment 8588901 [details]
Readerview_doorhanger_FTE (48x48)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> (In reply to Holly Habstritt Gaal [:Habber] from comment #11)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> > > Holly, can you provide the graphic mentioned in comment #5?
> > 
> > Sure, Jared. What are the dimension requirements for that area? I'm not sure
> > if my mock-ups are to scale with how the door hangers are built.
> 
> Can you provide them in 48x48 and 96x96px?

There you go, Jared. Could I see a screenshot of the doorhanger once the image is implemented to be sure I exported things for you correctly?
Thanks! Here's a screenshot with the 48x48 icon in it, http://screencast.com/t/d9EELFur8FP
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> Thanks! Here's a screenshot with the 48x48 icon in it,
> http://screencast.com/t/d9EELFur8FP

I've got that in the wrong panel according to https://bug1129536.bugzilla.mozilla.org/attachment.cgi?id=8574210 but the sizing and positioning of it is what it will look like when it is in the correct panel. The spec shows the graphic relatively larger than in my screenshot, but that just appears to be how the UI Tour panels have been implemented. I did file bug 1150163 about tweaking some of the layout of these panels.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> > Thanks! Here's a screenshot with the 48x48 icon in it,
> > http://screencast.com/t/d9EELFur8FP
> 
> I've got that in the wrong panel according to
> https://bug1129536.bugzilla.mozilla.org/attachment.cgi?id=8574210 but the
> sizing and positioning of it is what it will look like when it is in the
> correct panel. The spec shows the graphic relatively larger than in my
> screenshot, but that just appears to be how the UI Tour panels have been
> implemented. I did file bug 1150163 about tweaking some of the layout of
> these panels.

Thanks! Yea, that does look pretty small and like there could be room for a little bit larger image. How possible is it to use a larger image there? Would we not want to because it isn't to spec, and/or because it is a challenge from a dev perspective to have door hangers display different image sizes (ie: you'd need to create a new style or template).

Perhaps mmaslaney has an opinion on this one since he created the original spec.
Flags: needinfo?(mmaslaney)

Comment 19

2 years ago
Comment on attachment 8586909 [details] [diff] [review]
Patch v1.1

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

Apologies for the delay due to the public holidays here.

This doesn't look like it deals with the blacklisting described in comment #0. Was there some reason that was omitted?

::: browser/components/readinglist/sidebar.js
@@ +98,5 @@
>  
> +    let browserWindow = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                          .getInterface(Ci.nsIWebNavigation)
> +                          .QueryInterface(Ci.nsIDocShell).chromeEventHandler
> +                          .ownerDocument.defaultView;

Nit: indentation.

::: browser/components/uitour/UITour.jsm
@@ +189,5 @@
>      ["quit",        {query: "#PanelUI-quit"}],
> +    ["readerMode-urlBar", {query: "#reader-mode-button"}],
> +    ["readingList-header", {
> +      query: "#sidebar-box[sidebarcommand=readingListSidebar] > #sidebar-header > #sidebar-title",
> +      infoPanelPosition: "rightcenter topleft",

This won't work with RTL. Can't you use after_end or similar?
Attachment #8586909 - Flags: review?(gijskruitbosch+bugs) → review-
Flags: needinfo?(mmaslaney)
Created attachment 8589193 [details] [diff] [review]
Patch v1.2

(In reply to :Gijs Kruitbosch from comment #19)
> Comment on attachment 8586909 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 8586909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Apologies for the delay due to the public holidays here.
> 
> This doesn't look like it deals with the blacklisting described in comment
> #0. Was there some reason that was omitted?

Sorry, I missed that in comment #0. Instead of adding a blacklist for mozilla.org, I added a check to see if an info-panel was already open. As I understand it, we will want these to open up on a mozilla.org page (bug 1134501).

> ::: browser/components/uitour/UITour.jsm
> @@ +189,5 @@
> >      ["quit",        {query: "#PanelUI-quit"}],
> > +    ["readerMode-urlBar", {query: "#reader-mode-button"}],
> > +    ["readingList-header", {
> > +      query: "#sidebar-box[sidebarcommand=readingListSidebar] > #sidebar-header > #sidebar-title",
> > +      infoPanelPosition: "rightcenter topleft",
> 
> This won't work with RTL. Can't you use after_end or similar?

rightcenter and topleft are flipped for RTL by the code at http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsMenuPopupFrame.cpp#966. I didn't add a comment here because there are other places within UITour.jsm that make reference to these positions, such as loop-roomList.
Attachment #8586909 - Attachment is obsolete: true
Attachment #8589193 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8589193 [details] [diff] [review]
Patch v1.2

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

Gonna re-work how the tour is allowed instead of just checking to see if an info panel is open.
Attachment #8589193 - Flags: review?(gijskruitbosch+bugs)
Created attachment 8591155 [details] [diff] [review]
Patch v1.3
Attachment #8589193 - Attachment is obsolete: true
Attachment #8591155 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8591155 [details] [diff] [review]
Patch v1.3

(this applies on top of the patch from bug 1134501)

Comment 24

2 years ago
Comment on attachment 8591155 [details] [diff] [review]
Patch v1.3

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

OK, but in this case, there doesn't seem to be anything to prevent this thing from popping up when another panel is open (the thing your previous patch did in lieu of a mechanism to check if showing the panel was blocked/allowed)? Also, there doesn't seem to be any code actually ever setting the pref to true...

::: browser/components/readinglist/sidebar.js
@@ +74,5 @@
> +                              .QueryInterface(Ci.nsIDocShell).chromeEventHandler
> +                              .ownerDocument.defaultView;
> +    let selectedBrowser = browserWindow.gBrowser.selectedBrowser;
> +    if (!Services.prefs.getBoolPref("browser.readinglist.sidebarEverOpened") &&
> +        !selectedBrowser.currentURI.asciiHost.endsWith("mozilla.org")) {

I wonder if selectedBrowser.currentURI is ever null...

::: browser/modules/ReaderParent.jsm
@@ +159,5 @@
>        command.setAttribute("accesskey", gStringBundle.GetStringFromName("readerView.enter.accesskey"));
>      }
> +
> +    if (browser.isArticle &&
> +        !Services.prefs.getBoolPref("browser.reader.detectedFirstArticle") &&

I don't see you setting this pref anywhere, either here or in the other bug...

::: browser/themes/linux/jar.mn
@@ +93,5 @@
>    skin/classic/browser/welcome-back.svg                     (../shared/incontent-icons/welcome-back.svg)
>    skin/classic/browser/readerMode.svg                       (../shared/reader/readerMode.svg)
>    skin/classic/browser/readinglist/icons.svg          (../shared/readinglist/icons.svg)
>    skin/classic/browser/readinglist/readinglist-icon.svg (../shared/readinglist/readinglist-icon.svg)
> +  skin/classic/browser/readinglist/readinglist-tour.png     (../shared/readinglist/readinglist-tour.png)

No hidpi image? :-(
Attachment #8591155 - Flags: review?(gijskruitbosch+bugs) → review-
Created attachment 8591996 [details] [diff] [review]
Patch v1.4

Thanks, setting the pref now. I didn't add a check to see if we're showing a UITour panel already since this won't show them if it is on mozilla.org. The UITour API doesn't allow to specify multiple images (hidpi vs 1x dpi) so I'm only providing the hidpi image and letting it downscale for regular dpi.
Attachment #8591155 - Attachment is obsolete: true
Attachment #8591996 - Flags: review?(gijskruitbosch+bugs)

Comment 26

2 years ago
Comment on attachment 8591996 [details] [diff] [review]
Patch v1.4

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

This would be r+, but:

(In reply to (behind on reviews) Jared Wein [:jaws] (please needinfo? me) from comment #25)
> Created attachment 8591996 [details] [diff] [review]
> Patch v1.4
> 
> Thanks, setting the pref now. I didn't add a check to see if we're showing a
> UITour panel already since this won't show them if it is on mozilla.org.

I don't understand what you mean here. In particular, what are "this" and "them" ? Initially, I thought you might mean that "this patch" won't show "the reading list / reader mode info panels" on mozilla.org, but the patch seems to *only* show them on mozilla.org, unless I'm misreading it? (always possible!)

Can you clarify and adjust and/or re-request review?


> The
> UITour API doesn't allow to specify multiple images (hidpi vs 1x dpi) so I'm
> only providing the hidpi image and letting it downscale for regular dpi.

Ah, ok, wfm.
Attachment #8591996 - Flags: review?(gijskruitbosch+bugs)

Updated

2 years ago
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Created attachment 8593512 [details] [diff] [review]
Patch v1.5

Fixed issues and rebased on top of bug 1134501.
Attachment #8591996 - Attachment is obsolete: true
Attachment #8593512 - Flags: review?(gijskruitbosch+bugs)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f078fdd35a47

Comment 29

2 years ago
Comment on attachment 8593512 [details] [diff] [review]
Patch v1.5

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

r=me once you re-add the image and all the jar.mn changes from the previous patch.

CAUTION: if/when this is uplifted, PLEASE make sure you fix the windows parts to include the image in the aero part of the jar.mn as well. :-)
Attachment #8593512 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #29)
> r=me once you re-add the image and all the jar.mn changes from the previous
> patch.

The image and jar.mn changes were landed in bug 1134501.

Comment 31

2 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #30)
> (In reply to :Gijs Kruitbosch from comment #29)
> > r=me once you re-add the image and all the jar.mn changes from the previous
> > patch.
> 
> The image and jar.mn changes were landed in bug 1134501.

D'oh. I looked in MXR, it clearly hadn't been updated yet.
Created attachment 8594163 [details] [diff] [review]
Patch for check-in
Attachment #8593512 - Attachment is obsolete: true
Keywords: checkin-needed
(Reporter)

Comment 33

2 years ago
Just met with Cory and Holly about UITour -- some of this patch will no longer be needed, because there won't be a reading list sidebar. But we do want to keep the infopanel to point out the reader view button the first time it appears in the url bar.

Two further changes, though:
* We want to swap the strings with a later panel, which will be more appropriate to just having Reader View but no Reader List
* We want to update the image in the panel because there's no Reader List "+" button.
Keywords: checkin-needed
(Reporter)

Comment 34

2 years ago
NI Holly for the updated icon.

Matt said he can finish this patch since he has context, and Jared's picking up the Pocket work.
Flags: needinfo?(hhabstritt.bugzilla)
Created attachment 8594261 [details] [diff] [review]
Patch v1.7 - Subset of 1.6 with changes to referenced strings (still needs image swap)

We're switching to using the strings from a later tour step since they don't reference the reading list.

This still needs the 2 images from Holly swapped to remove the plus icon.
Attachment #8566349 - Attachment is obsolete: true
Attachment #8588899 - Attachment is obsolete: true
Attachment #8588901 - Attachment is obsolete: true
Attachment #8594163 - Attachment is obsolete: true
Attachment #8594261 - Flags: review?(gijskruitbosch+bugs)
Summary: Implement infopanels to promote Reader View / Reading List when first seen → Implement infopanel to promote Reader View when first available

Updated

2 years ago
Attachment #8594261 - Flags: review?(gijskruitbosch+bugs) → review+
Created attachment 8594721 [details]
Reader View intro door hanger image 1x
Flags: needinfo?(hhabstritt.bugzilla)
Created attachment 8594722 [details]
Reader View intro door hanger image @2x
(In reply to Matthew N. [:MattN] from comment #35)
> Created attachment 8594261 [details] [diff] [review]
> Patch v1.7 - Subset of 1.6 with changes to referenced strings (still needs
> image swap)
> 
> We're switching to using the strings from a later tour step since they don't
> reference the reading list.
> 
> This still needs the 2 images from Holly swapped to remove the plus icon.

I attached the door hanger images in the requested size. However, per comment 16 and comment 17, is it possible to implement a larger, more readable image in this space? If that it is possible to do so, I can attach new assets.
(In reply to Holly Habstritt Gaal [:Habber] from comment #38)
> I attached the door hanger images in the requested size. However, per
> comment 16 and comment 17, is it possible to implement a larger, more
> readable image in this space? If that it is possible to do so, I can attach
> new assets.

They were always 48x48px AFAIK (I believe according to spec) and I'd rather not change that in this bug. The images you uploaded aren't the exact proper dimensions so they would get stretched. Could you attach ones that are 48x48 and 96x96?
Flags: needinfo?(hhabstritt.bugzilla)

Updated

2 years ago
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
(In reply to Matthew N. [:MattN] from comment #39)
> (In reply to Holly Habstritt Gaal [:Habber] from comment #38)
> > I attached the door hanger images in the requested size. However, per
> > comment 16 and comment 17, is it possible to implement a larger, more
> > readable image in this space? If that it is possible to do so, I can attach
> > new assets.
> 
> They were always 48x48px AFAIK (I believe according to spec) and I'd rather
> not change that in this bug. The images you uploaded aren't the exact proper
> dimensions so they would get stretched. Could you attach ones that are 48x48
> and 96x96?

I thought that was max size, not exact dimensions. I'll attache new assets.
Flags: needinfo?(hhabstritt.bugzilla)
Created attachment 8598967 [details]
Reader view intro door hanger @2x.png
Attachment #8594722 - Attachment is obsolete: true
Created attachment 8598968 [details]
Reader view intro door hanger.png
Attachment #8594721 - Attachment is obsolete: true
Created attachment 8599442 [details] [diff] [review]
Patch v1.8 - Subset of 1.6 with changes to referenced strings with image swap

Thanks Holly.

In testing this I realized there is more work to do since it doesn't handle hiding of the info panel when the reader icon disappears (e.g. via back-forward navigation) and this causes the panel to appear at unusual locations. This is another case where bug 1109868 would make this painless.
Attachment #8594261 - Attachment is obsolete: true
(In reply to Matthew N. [:MattN] from comment #43)
> Created attachment 8599442 [details] [diff] [review]
> Patch v1.8 - Subset of 1.6 with changes to referenced strings with image swap
> 
> Thanks Holly.
> 
> In testing this I realized there is more work to do since it doesn't handle
> hiding of the info panel when the reader icon disappears (e.g. via
> back-forward navigation) and this causes the panel to appear at unusual
> locations. This is another case where bug 1109868 would make this painless.

Thanks for the update, Matt. Any concerns of this and bug 1109868 landing in 38.1?
(In reply to Holly Habstritt Gaal [:Habber] from comment #44)
> Thanks for the update, Matt. Any concerns of this and bug 1109868 landing in
> 38.1?

Bug 1109868 is going to be too risky for 38 so we're going to have to implement another workaround for that bug :( It should still be do-able for 38.0.5.
Created attachment 8599550 [details] [diff] [review]
Patch v2 - Handle the anchor disappearing and add tests

Now with tests!

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=175a4601a0a4
Assignee: jaws → MattN+bmo
Attachment #8599442 - Attachment is obsolete: true
Attachment #8599550 - Flags: review?(jaws)
Comment on attachment 8599550 [details] [diff] [review]
Patch v2 - Handle the anchor disappearing and add tests

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

::: browser/base/content/test/general/browser_readerMode.js
@@ +31,5 @@
>    // Set required test prefs.
>    TEST_PREFS.forEach(([name, value]) => {
>      Services.prefs.setBoolPref(name, value);
>    });
> +  Services.prefs.setBoolPref("browser.reader.detectedFirstArticle", false);

Should we set this pref to `true` for the test suite so that the UI Tour doesn't get triggered if a different test loads a page that is "readerable"?

::: browser/modules/ReaderParent.jsm
@@ +244,5 @@
>      let targetPromise = UITour.getTarget(win, "readerMode-urlBar");
>      targetPromise.then(target => {
>        let browserBundle = Services.strings.createBundle("chrome://browser/locale/browser.properties");
> +      let icon = "chrome://browser/skin/";
> +      if (AppConstants.platform == "macosx" && win.devicePixelRatio > 1) {

Windows is migrating to using @2x images also, so I'd like us to leave the platform detection out of this and just check the dppx.

::: browser/themes/osx/jar.mn
@@ +145,5 @@
>    skin/classic/browser/session-restore.svg            (../shared/incontent-icons/session-restore.svg)
>    skin/classic/browser/tab-crashed.svg                (../shared/incontent-icons/tab-crashed.svg)
>    skin/classic/browser/welcome-back.svg               (../shared/incontent-icons/welcome-back.svg)
>    skin/classic/browser/reader-tour.png                (../shared/reader/reader-tour.png)
> +  skin/classic/browser/reader-tour@2x.png             (../shared/reader/reader-tour@2x.png)

Can you add this to linux and windows, as well as the related CSS for the @2x image?
Attachment #8599550 - Flags: review?(jaws) → review+

Comment 48

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/6404e6d64787
Flags: in-testsuite+
Whiteboard: [reader-ui] → [reader-ui][fixed-in-fx-team]
Created attachment 8600082 [details] [diff] [review]
Patch v2.1 for uplift - Address comments. r=jaws/Gijs

Approval Request Comment
[Feature/regressing bug #]: Panel to introduce the Reader View feature in 38.0.5
[User impact if declined]: Users won't be introduce to Reader View
[Describe test coverage new/current, TreeHerder]: b-c test
[Risks and why]: Low risk, one-time panel with a pref
[String/UUID change made/needed]: None. Strings were pre-landed

This needs to land on 38.0.5 but I'm not sure what the proper branches and stuff are so please correct the flags.
Attachment #8599550 - Attachment is obsolete: true
Attachment #8600082 - Flags: review+
Attachment #8600082 - Flags: checkin+
Attachment #8600082 - Flags: approval-mozilla-beta?
Attachment #8600082 - Flags: approval-mozilla-aurora?
status-firefox37: --- → unaffected
status-firefox38: --- → unaffected
status-firefox38.0.5: --- → affected
status-firefox39: --- → affected
status-firefox40: --- → fixed
(Assignee)

Comment 50

2 years ago
verification-steps
For QE: This can land on 38.0.0 but it shouldn't appear since my understanding is that reader.parse-on-load.enabled will be false on that version. We only want to introduce this feature for 38.0.5 and later.

The panel should appear once and then toggle browser.reader.detectedFirstArticle to true to remember that it shouldn't show the panel again.
status-firefox38: unaffected → affected
The 38.0.5 flag is enough for this
status-firefox38: affected → unaffected
https://hg.mozilla.org/mozilla-central/rev/6404e6d64787
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Whiteboard: [reader-ui][fixed-in-fx-team] → [reader-ui]
Target Milestone: --- → Firefox 40
Attachment #8600082 - Flags: approval-mozilla-beta?
Attachment #8600082 - Flags: approval-mozilla-beta+
Attachment #8600082 - Flags: approval-mozilla-aurora?
Attachment #8600082 - Flags: approval-mozilla-aurora+
Depends on: 1161014
Depends on: 1161017
Depends on: 1161018
Depends on: 1161022
Confirmed fixed on Nightly 40.0a1 (2015-05-03), using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5. 

Testing was performed with and without e10s, with 4 new bugs found and filed: Bug 1161014, Bug 1161017, Bug 1161018 and Bug 1161022.
Status: RESOLVED → VERIFIED
status-firefox40: fixed → verified

Updated

2 years ago
Blocks: 1161073
Lots of conflicts on Aurora. Note there's a pretty good backlog of 38.0.5 uplifts at the moment due to various conflicts.
Depends on: 1160584
It still applies cleanly to me with no fuzz on Aurora tip.
Flags: needinfo?(ryanvm)
Probably because Margaret uplifted bug 1154028 in the mean time :)
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/releases/mozilla-aurora/rev/20bcf9c3e9b7
status-firefox39: affected → fixed
https://hg.mozilla.org/releases/mozilla-release/rev/f53c601dafa3
status-firefox38.0.5: affected → fixed
Depends on: 1163526
I verified this bug using the following environment:

FF 38.0.5
Build Id:20150511143336
OS: Win 7 x64, Mac Os X 10.9.5, Ubuntu 14.04 x86
status-firefox38.0.5: fixed → verified
Removing qe-verify flag as verification on Firefox 38.0.5 and 40 should suffice.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.