Closed
Bug 1134507
Opened 10 years ago
Closed 10 years ago
Implement infopanel to promote Reader View when first available
Categories
(Firefox :: Tours, defect, P1)
Firefox
Tours
Tracking
()
Tracking | Status | |
---|---|---|
firefox37 | --- | unaffected |
firefox38 | --- | unaffected |
firefox38.0.5 | --- | verified |
firefox39 | --- | fixed |
firefox40 | --- | verified |
People
(Reporter: Dolske, Assigned: MattN)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [reader-ui])
Attachments
(3 files, 16 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
MattN
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
MattN
:
checkin+
|
Details | Diff | Splinter Review |
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•10 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•10 years ago
|
Blocks: fx-UITour-ReadingList
Comment 2•10 years ago
|
||
Required for Campaign Release
Assignee | ||
Comment 3•10 years ago
|
||
(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•10 years ago
|
Points: --- → 5
Reporter | ||
Updated•10 years ago
|
Priority: -- → P1
Comment 4•10 years ago
|
||
The most up to date wireframe for the tour will be found in the nonobsolete attachment in bug 1129536.
Updated•10 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: andrei.vaida
Updated•10 years ago
|
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Updated•10 years ago
|
Iteration: --- → 40.1 - 13 Apr
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
Attachment #8586906 -
Attachment is obsolete: true
Attachment #8586906 -
Flags: review?(bmcbride)
Attachment #8586909 -
Flags: review?(bmcbride)
Updated•10 years ago
|
Attachment #8586909 -
Flags: review?(bmcbride) → review?(gijskruitbosch+bugs)
Comment 10•10 years ago
|
||
Holly, can you provide the graphic mentioned in comment #5?
Flags: needinfo?(hhabstritt.bugzilla)
Comment 11•10 years ago
|
||
(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)
Comment 12•10 years ago
|
||
(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)
Updated•10 years ago
|
Whiteboard: [reader-ui]
Comment 13•10 years ago
|
||
Flags: needinfo?(hhabstritt.bugzilla)
Comment 14•10 years ago
|
||
Comment 15•10 years ago
|
||
(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?
Comment 16•10 years ago
|
||
Thanks! Here's a screenshot with the 48x48 icon in it, http://screencast.com/t/d9EELFur8FP
Comment 17•10 years ago
|
||
(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.
Comment 18•10 years ago
|
||
(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•10 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-
Updated•10 years ago
|
Flags: needinfo?(mmaslaney)
Comment 20•10 years ago
|
||
(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 21•10 years ago
|
||
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)
Comment 22•10 years ago
|
||
Attachment #8589193 -
Attachment is obsolete: true
Attachment #8591155 -
Flags: review?(gijskruitbosch+bugs)
Comment 23•10 years ago
|
||
Comment on attachment 8591155 [details] [diff] [review]
Patch v1.3
(this applies on top of the patch from bug 1134501)
Comment 24•10 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-
Comment 25•10 years ago
|
||
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•10 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•10 years ago
|
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Comment 27•10 years ago
|
||
Fixed issues and rebased on top of bug 1134501.
Attachment #8591996 -
Attachment is obsolete: true
Attachment #8593512 -
Flags: review?(gijskruitbosch+bugs)
Comment 28•10 years ago
|
||
Comment 29•10 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+
Comment 30•10 years ago
|
||
(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•10 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.
Comment 32•10 years ago
|
||
Attachment #8593512 -
Attachment is obsolete: true
Updated•10 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 33•10 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•10 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)
Assignee | ||
Comment 35•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Summary: Implement infopanels to promote Reader View / Reading List when first seen → Implement infopanel to promote Reader View when first available
Updated•10 years ago
|
Attachment #8594261 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 36•10 years ago
|
||
Flags: needinfo?(hhabstritt.bugzilla)
Comment 37•10 years ago
|
||
Comment 38•10 years ago
|
||
(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.
Assignee | ||
Comment 39•10 years ago
|
||
(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•10 years ago
|
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
Comment 40•10 years ago
|
||
(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)
Comment 41•10 years ago
|
||
Attachment #8594722 -
Attachment is obsolete: true
Comment 42•10 years ago
|
||
Attachment #8594721 -
Attachment is obsolete: true
Assignee | ||
Comment 43•10 years ago
|
||
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
Comment 44•10 years ago
|
||
(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?
Assignee | ||
Comment 45•10 years ago
|
||
(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.
Assignee | ||
Comment 46•10 years ago
|
||
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 47•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Flags: in-testsuite+
Whiteboard: [reader-ui] → [reader-ui][fixed-in-fx-team]
Assignee | ||
Comment 49•10 years ago
|
||
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?
Assignee | ||
Updated•10 years ago
|
status-firefox37:
--- → unaffected
status-firefox38:
--- → unaffected
status-firefox38.0.5:
--- → affected
status-firefox39:
--- → affected
status-firefox40:
--- → fixed
Assignee | ||
Comment 50•10 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.
Comment 51•10 years ago
|
||
The 38.0.5 flag is enough for this
Comment 52•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [reader-ui][fixed-in-fx-team] → [reader-ui]
Target Milestone: --- → Firefox 40
Updated•10 years ago
|
Attachment #8600082 -
Flags: approval-mozilla-beta?
Attachment #8600082 -
Flags: approval-mozilla-beta+
Attachment #8600082 -
Flags: approval-mozilla-aurora?
Attachment #8600082 -
Flags: approval-mozilla-aurora+
Comment 53•10 years ago
|
||
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
Comment 54•10 years ago
|
||
Lots of conflicts on Aurora. Note there's a pretty good backlog of 38.0.5 uplifts at the moment due to various conflicts.
Assignee | ||
Comment 55•10 years ago
|
||
It still applies cleanly to me with no fuzz on Aurora tip.
Flags: needinfo?(ryanvm)
Comment 56•10 years ago
|
||
Probably because Margaret uplifted bug 1154028 in the mean time :)
Flags: needinfo?(ryanvm)
Comment 57•10 years ago
|
||
Comment 58•10 years ago
|
||
Updated•10 years ago
|
Comment 59•10 years ago
|
||
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
Comment 60•10 years ago
|
||
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.
Description
•