Closed Bug 1126967 Opened 9 years ago Closed 8 years ago

[reader mode] [UX] improve the loading transition

Categories

(Toolkit :: Reader Mode, defect, P4)

defect
Points:
5

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: clarkbw, Assigned: danhuang, Mentored)

References

Details

(Whiteboard: [outreachy-12])

Attachments

(1 file, 6 obsolete files)

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
Flags: firefox-backlog+
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.
Priority: -- → P4
Status: NEW → ASSIGNED
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.
Flags: needinfo?(bwinton)
So you were thinking of bouncing in from the left, starting with the toolbar?  (Or ending with the toolbar?)
Flags: needinfo?(bwinton)
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Reopening this for Version 2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: mmaslaney → jeroentulp
Assignee: jeroentulp → nobody
Component: General → Reader Mode
Product: Firefox → Toolkit
Depends on: 1184950
No longer depends on: 1184950
Blocks: fx-qx
Mentor: jaws
Whiteboard: [outreachy-12]
Points: --- → 1
Points: 1 → 5
Status: REOPENED → NEW
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.
Flags: needinfo?(jaws)
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.
Assignee: nobody → dhuang
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
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.
Attachment #8746966 - Flags: review?(jaws)
Hi Jared,

This patch add transition to reader toolbar when the content loaded.  
Please help review this patch.
Attachment #8746970 - Flags: review?(jaws)
Attachment #8746966 - Attachment is patch: true
Attachment #8746970 - Attachment is patch: true
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
Attachment #8746966 - Flags: review?(jaws) → review-
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)
Attachment #8746970 - Flags: review?(jaws) → review-
(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.
Attachment #8746966 - Attachment is obsolete: true
Attachment #8748992 - Flags: review?(jaws)
Hi Jared,

Please help review this patch.
This patch update css style from .toolbar:not([loaded]) to body:not(.loaded).
Attachment #8746970 - Attachment is obsolete: true
Hi Jared,

Please help review this patch.
This patch add the test case for observing the attribute change when switching reader mode.
Attachment #8748998 - Flags: review?(jaws)
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).
Attachment #8748993 - Flags: review?(jaws)
Attachment #8748993 - Flags: review?(jaws)
Attachment #8748993 - Attachment is obsolete: true
Attachment #8749028 - Flags: review?(jaws)
Attachment #8749028 - Attachment description: part 2_v2 - add transition to reader toolbar when the content loaded → patch: part 2_v2 - add transition to reader toolbar when the content 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.
Attachment #8748992 - Attachment is patch: true
Attachment #8748998 - Attachment is patch: true
Attachment #8749028 - Attachment is patch: true
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.
Attachment #8748992 - Flags: review?(jaws) → review+
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".
Attachment #8748998 - Flags: review?(jaws) → review+
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.
Attachment #8749028 - Flags: review?(jaws) → review+
(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?
Flags: needinfo?(jaws)
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!
Flags: needinfo?(jaws)
> 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
Attached patch PatchSplinter Review
try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=21c2dc7e426d
Attachment #8748992 - Attachment is obsolete: true
Attachment #8748998 - Attachment is obsolete: true
Attachment #8749028 - Attachment is obsolete: true
Attachment #8752652 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/71e53cab38fd
Status: ASSIGNED → RESOLVED
Closed: 9 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: