The implementation might be connected to bug 1113795, not sure but it currently flashes the "Loading..." text off that page which I'm not sure we want to keep. Lets improve the transition from the page to reader mode by either improving the loading text or working in a transition. For Android Ian had done some work in bug 872046 which is useful for reference.

For reference here's an animation that safari on mac os x uses: http://cl.ly/1u0d3I310u15

I think part of the problem you're seeing here is bug 1130206. When you hit the reader mode button, we should already have the article, so there shouldn't actually be any wait time where we need to show "Loading..." (that should only happen when we're downloading the article, like if you just navigated directly to a reader mode page). However, I'd like to explore what it would be like to animate the reader mode content over the original page, as opposed to actually loading a new page (like what Safari does). I worry this might be tricky to implement, but a prototype could help explore how hard that is.

For reference, onthe Android side, bug 872046 was actually a duplicate of bug 1114708 (where mocks are) but that works not currently being tracked AFAIK.

Blake, I was picturing an animation much like the addon Clearly utilizes: See here: http://cl.ly/0R1V1G1M0f43 There's some slight ease in and ease out with a little bounce back at the end.

So you were thinking of bouncing in from the left, starting with the toolbar? (Or ending with the toolbar?)

So, I think this is going to be "hard"… Because we're loading the reader-mode version of the page in the same browser (like if we clicked a link), the new content replaces the old content, and so we don't really have a good way to transition between them. :( Margaret or Jaws might have a better idea of how we could make this work (even in a hacky-prototype kinda way), but as far as I can tell, there's not really a good way for us to do it in production.

(In reply to Blake Winton (:bwinton) from comment #6) > So, I think this is going to be "hard"… > Because we're loading the reader-mode version of the page in the same > browser (like if we clicked a link), the new content replaces the old > content, and so we don't really have a good way to transition between them. > :( > > Margaret or Jaws might have a better idea of how we could make this work > (even in a hacky-prototype kinda way), but as far as I can tell, there's not > really a good way for us to do it in production. Yeah, I think this is too hard to do for v1, since I think this would require something like loading about:reader in a new window (or frame), and placing that on top of the original content. We've been punting on doing something this on mobile for a long time, since we don't really support drawing multiple windows. I understand the focus right now is on small wins we can make for Fx38, but at some point we should try to find a real solution for this.

Reopening this for Version 2

Hi Jared, I'm interested in this bug, and would like to work on this. But I didn't understand what is the 'Version 2' means in Comment 8. It would be grateful if you could guide me here.

The "version 2" mentioned in comment 8 was merely referring to future enhancements to reader mode. Using the term "version 2" is indeed confusing, as we have made many small improvements over time and haven't considered them as part of a "new version" of Reader Mode. So to give more background here and an update from when the bug was filed: When reader mode is opened from a page that is already loaded, the content of the loaded page is used to render reader mode, so there is no network request required. This means that the page loads pretty fast, and we generally don't need to worry about implementing a "Loading..." graphic. However, there are a couple things that can be done here: 1) Notice that if you entering and exiting reader mode causes the Reader Mode icon in the toolbar to hide momentarily. This shouldn't hide or flicker when transitioning to or from Reader Mode. 2) We can transition the Reader Mode sidebar when Reader Mode loads. Let's start with the following: > aboutReaderControls.css > .toolbar { > transition: margin 0.3s ease-out; > ... > } > > .toolbar:not([loaded]):-moz-locale-dir(ltr) { > margin-left: -40px; > } > > .toolbar:not([loaded]):-moz-locale-dir(rtl) { > margin-right: -40px; > } Then we can set the [loaded] attribute on the .toolbar when the `domcontentloaded` event fires.

Thanks for your comments. It's really helpful!

Hi Jared, This patch keep reader mode icon showing when leaving reader mode from the reader mode enabled. This can prevent reader icon flicker. Please help review this patch.

Hi Jared, This patch add transition to reader toolbar when the content loaded. Please help review this patch.

Comment on attachment 8746966 [details] [diff] [review] patch: part 1 - keep reader mode icon showing when leaving reader mode Review of attachment 8746966 [details] [diff] [review]: ----------------------------------------------------------------- Can you please write a test that does the following: 1. Use a mutation observer (https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver) to track whether the "hidden" attribute is changed on the "View:ReaderView" command element? 2. Confirm the command has the hidden attribute 3. Open a document that triggers reader mode button to show 4. Confirm that the mutation observer was triggered and the "hidden" attribute is removed 5. Navigate to a document that triggers the button to hide 6. Confirm that the mutation observer was triggered and the "hidden" attribute is added 7. Navigate to the document that triggers the button to show 8. Confirm that the mutation observer was triggered and the "hidden" attribute is removed 9. Enter Reader Mode 10. Confirm that the mutation observer was triggered but only the "readeractive" attribute is added 11. Exit Reader Mode 12. Confirm that the mutation observer was triggered but only the "readeractive" attribute is removed 13. Navigate to the page that doesn't trigger Reader Mode 14. Confirm that the mutation observer was triggered and the "hidden" attribute is added ::: browser/base/content/tab-content.js @@ +245,5 @@ > var AboutReaderListener = { > > _articlePromise: null, > > + _isArticle: false, Please rename this to _isLeavingReaderMode

Comment on attachment 8746970 [details] [diff] [review] patch: part 2 - add transition to reader toolbar when the content loaded Review of attachment 8746970 [details] [diff] [review]: ----------------------------------------------------------------- Bug1126967.part1 should not be part of this patch. ::: toolkit/components/reader/AboutReader.jsm @@ +605,5 @@ > this._updateImageMargins(); > > this._requestFavicon(); > this._doc.body.classList.add("loaded"); > + this._toolbarElement.setAttribute("loaded", true); Since the "loaded" class is added to the body, we don't need to add it to the toolbarElement. This line can be removed. ::: toolkit/themes/shared/aboutReaderControls.css @@ +85,5 @@ > + transform: translateX(0); > + transition: transform 0.3s ease-out; > +} > + > +.toolbar:not([loaded]):-moz-locale-dir(ltr) { This selector can be changed to body:not(.loaded) .toolbar:-moz-locale-dir(ltr) @@ +89,5 @@ > +.toolbar:not([loaded]):-moz-locale-dir(ltr) { > + transform: translateX(-100%); > +} > + > +.toolbar:not([loaded]):-moz-locale-dir(rtl) { This selector can be changed to body:not(.loaded) .toolbar:-moz-locale-dir(rtl)

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #15) > Comment on attachment 8746966 [details] [diff] [review] > patch: part 1 - keep reader mode icon showing when leaving reader mode > > Review of attachment 8746966 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can you please write a test that does the following: > 1. Use a mutation observer > (https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver) to track > whether the "hidden" attribute is changed on the "View:ReaderView" command > element? > 2. Confirm the command has the hidden attribute > 3. Open a document that triggers reader mode button to show > 4. Confirm that the mutation observer was triggered and the "hidden" > attribute is removed > 5. Navigate to a document that triggers the button to hide > 6. Confirm that the mutation observer was triggered and the "hidden" > attribute is added > 7. Navigate to the document that triggers the button to show > 8. Confirm that the mutation observer was triggered and the "hidden" > attribute is removed > 9. Enter Reader Mode > 10. Confirm that the mutation observer was triggered but only the > "readeractive" attribute is added > 11. Exit Reader Mode > 12. Confirm that the mutation observer was triggered but only the > "readeractive" attribute is removed > 13. Navigate to the page that doesn't trigger Reader Mode > 14. Confirm that the mutation observer was triggered and the "hidden" > attribute is added Thanks for your review comment. I will create another patch for this test case.

Hi Jared, Please help review this patch. This patch rename _isArticle to _isLeavingReaderMode.

Hi Jared, Please help review this patch. This patch update css style from .toolbar:not([loaded]) to body:not(.loaded).

Hi Jared, Please help review this patch. This patch add the test case for observing the attribute change when switching reader mode.

Comment on attachment 8748993 [details] patch: part 2_v2 - add transition to reader toolbar when the content loaded Hi Jared, Please help review this patch. This patch update css style from .toolbar:not([loaded]) to body:not(.loaded).

Hi Dan, I will review your patches as you have uploaded them now, but next time please fold all of your patches together, and mark them as a "patch" when you upload them. If the attachment is not marked as a "patch", then the review tools won't be enabled for the attachment.

Comment on attachment 8748992 [details] [diff] [review] patch: part 1_v2 - keep reader mode icon showing when leaving reader mode Review of attachment 8748992 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/tab-content.js @@ +303,5 @@ > break; > > case "pagehide": > this.cancelPotentialPendingReadabilityCheck(); > + sendAsyncMessage("Reader:UpdateReaderButton", { isArticle: this._isLeavingReaderMode }); We should add a comment above the sendAsyncMessage line to explain what "_isLeavingReaderMode" is. // this._isLeavingReaderMode is used here to keep the Reader Mode icon // visible in the location bar when transitioning from reader-mode page // back to the source page.

Comment on attachment 8748998 [details] [diff] [review] patch: part 3_v1 - add test case for observing attribute transform Review of attachment 8748998 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/general/browser_readerMode.js @@ +111,5 @@ > + > + function observeAttribute(element, attribute, value, triggerFn, checkFn) { > + return new Promise(resolve => { > + let observer = new MutationObserver(() => { > + if (element.getAttribute(attribute) === value) { Why do we need this if-condition here? It seems that it duplicates the work that will be done in the checkFn() function. @@ +128,5 @@ > + }); > + }; > + > + var command = document.getElementById("View:ReaderView"); > + var tab = gBrowser.selectedTab = gBrowser.addTab(); Replace this with: let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser); and also please use `let` instead of `var`. @@ +129,5 @@ > + }; > + > + var command = document.getElementById("View:ReaderView"); > + var tab = gBrowser.selectedTab = gBrowser.addTab(); > + isnot(command.hidden, null, "Command element should have the hidden attribute"); This should use: is(command.hidden, true, "Command element should have the hidden attribute"); @@ +175,5 @@ > + () => { > + readerButton.click(); > + }, > + () => { > + is(readerButton.getAttribute("readeractive"), "", "Command's hidden attribute should be false on a reader-able page"); This should be "readerButton's readeractive attribute should be empty when reader mode is exited".

Comment on attachment 8749028 [details] [diff] [review] patch: part 2_v2 - add transition to reader toolbar when the content loaded Review of attachment 8749028 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/themes/shared/aboutReader.css @@ +126,5 @@ > +/* Add toolbar transition base on loaded class */ > + > +body.loaded .toolbar { > + transition: transform 0.3s ease-out; > + transform: translateX(0); We can remove the `transform` rule here, as once the .loaded class is added to the body element, the rules below will no longer apply and transform will be set to its initial value.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23) > Hi Dan, I will review your patches as you have uploaded them now, but next > time please fold all of your patches together, and mark them as a "patch" > when you upload them. If the attachment is not marked as a "patch", then the > review tools won't be enabled for the attachment. Got it! Thanks for your kind reminder.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #25) > Comment on attachment 8748998 [details] [diff] [review] > patch: part 3_v1 - add test case for observing attribute transform > > Review of attachment 8748998 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/test/general/browser_readerMode.js > @@ +111,5 @@ > > + > > + function observeAttribute(element, attribute, value, triggerFn, checkFn) { > > + return new Promise(resolve => { > > + let observer = new MutationObserver(() => { > > + if (element.getAttribute(attribute) === value) { > > Why do we need this if-condition here? It seems that it duplicates the work > that will be done in the checkFn() function. > When use the MutationObserver to observe command's hidden attribute, the callback will be called on each mutation even though the attribute didn't change. For example, in the test case: > 3. Open a document that triggers reader mode button to show The hidden attribute is true in the beginning. Then the tab load the url, this would trigger the first mutation's callback, but the hidden attribute is still true. Later, after make sure the url is available for reader mode, update the hidden attribute to be false and trigger the second mutation's callback. The checkFn should be execute after the attribute changes, so I would update observeAttribute() as the following: function observeAttribute(element, attribute, triggerFn, checkFn) { return new Promise(resolve => { let observer = new MutationObserver((mutations) => { mutations.forEach( mu => { if(element.getAttribute(attribute) !== mu.oldValue) { checkFn(); resolve(); observer.disconnect(); } }); }); observer.observe(element, { attributes: true, attributeOldValue: true, attributeFilter: [attribute] }); triggerFn(); }); }; Do you think this is better than the previous version?

Comment on attachment 8748992 [details] [diff] [review] patch: part 1_v2 - keep reader mode icon showing when leaving reader mode Review of attachment 8748992 [details] [diff] [review]: ----------------------------------------------------------------- Could you file a Firefox for Android bug to make a similar change on mobile as well? We have similar logic in /mobile/android/chrome/content/content.js.

(In reply to Dan Huang[:danhuang] from comment #28) > Do you think this is better than the previous version? Yeah, I like that better. Nice!

> Could you file a Firefox for Android bug to make a similar change on mobile > as well? We have similar logic in /mobile/android/chrome/content/content.js. Sure! Bug 1271511

try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=21c2dc7e426d