Last Comment Bug 778489 - Implement nice transition to Reader Mode
: Implement nice transition to Reader Mode
Status: VERIFIED FIXED
ui-responsiveness
:
Product: Firefox for Android
Classification: Client Software
Component: Reader View (show other bugs)
: Trunk
: ARM Android
: P1 normal (vote)
: Firefox 18
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: reader
  Show dependency treegraph
 
Reported: 2012-07-28 14:38 PDT by Peter Retzer (:pretzer)
Modified: 2012-09-19 02:00 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified


Attachments
Screenshot of the issue (68.34 KB, image/png)
2012-07-28 14:38 PDT, Peter Retzer (:pretzer)
no flags Details
Use ReaderModeUtils to create about:reader urls (4.80 KB, patch)
2012-09-07 12:03 PDT, Lucas Rocha (:lucasr)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review
Don't show progress when entering about:reader (6.18 KB, patch)
2012-09-07 12:04 PDT, Lucas Rocha (:lucasr)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review
Keep title and favicon when entering reader mode (6.23 KB, patch)
2012-09-07 12:05 PDT, Lucas Rocha (:lucasr)
no flags Details | Diff | Splinter Review
Keep title and favicon when entering reader mode (8.36 KB, patch)
2012-09-10 07:04 PDT, Lucas Rocha (:lucasr)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review
Fade Reader content once article is loaded (6.34 KB, patch)
2012-09-11 08:03 PDT, Lucas Rocha (:lucasr)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review
Fix misc aboutReader.css warnings (1.58 KB, patch)
2012-09-11 08:03 PDT, Lucas Rocha (:lucasr)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Peter Retzer (:pretzer) 2012-07-28 14:38:07 PDT
Created attachment 646906 [details]
Screenshot of the issue

When entering reader mode the awesomebar content changes from the site's title to the 'about:reader' url while loading the article (see screenshot). Once loading is finished the awesomebar content changes back to the site's title that was visible before.
It would be nice if the title could stick while loading reader mode.
Comment 1 Daniel Lombraña González 2012-08-15 00:25:28 PDT
I confirm this bug.
Comment 2 Lucas Rocha (:lucasr) 2012-08-25 03:55:44 PDT
The transition to reader mode should not feel clunky. I think have proper design for thetransition: not only title and favicon but also for the content area.
Comment 3 Lucas Rocha (:lucasr) 2012-08-31 11:58:42 PDT
Renaming bug to cover wider topic of transitioning from page to reader mode. I have a prototype that makes the transition feel much better. What it does:
- It doesn't show transitory URL on toolbar when loading
- It doesn't show progress spinner before loading reader mode (just like about:home)
- It keeps the page title stable on the transition (which makes sense given that reader mode is just a different presentation for the same page)
- Fades Reader content in using a quick CSS transition

This is subtle enough to not feel overdone and cool enough to feel, well, cool :-)

Here's a test build: https://dl.dropbox.com/u/1187037/reader-transition.apk

Ian, Madhava, what do you think? I have a kind of hacky patch right now but it shouldn't be hard to clean it up.
Comment 4 Lucas Rocha (:lucasr) 2012-08-31 12:01:59 PDT
Ian, Madhava, feedback is welcome.
Comment 5 Daniel Lombraña González 2012-09-07 00:53:57 PDT
Hi Lucas,

I tested it today and it works really well. Actually, the new mode seems to convert the page faster than before, so congratulations for this patch! Do you know when will you merge it?

Cheers,

Daniel
Comment 6 Lucas Rocha (:lucasr) 2012-09-07 02:12:35 PDT
(In reply to Daniel Lombraña González from comment #5)
> I tested it today and it works really well. Actually, the new mode seems to
> convert the page faster than before, so congratulations for this patch! Do
> you know when will you merge it?

Interesting. This patch shouldn't really affect how fast reader mode starts. It will affect how you perceive the transition from page to reader though. Seems to be working then :-)
Comment 7 Daniel Lombraña González 2012-09-07 02:35:34 PDT
Maybe it seems to load faster because you do not actually see the transition in the URL bar. I mean, in the previous version when you fired the reader mode, the URL changed, and the spinner or throbber started, then you see the reader version. In this new one you go directly to this, so maybe this could be a possible cause. In any case, thanks a lot for this awesome work! I'm using it almost every day as it makes it really useful to read the web (I'm thinking that it will be "interesting" to have this mode by default to load pages or entire web sites like news papers, so you actually read :-D)
Comment 8 Lucas Rocha (:lucasr) 2012-09-07 12:03:47 PDT
Created attachment 659310 [details] [diff] [review]
Use ReaderModeUtils to create about:reader urls
Comment 9 Lucas Rocha (:lucasr) 2012-09-07 12:04:40 PDT
Created attachment 659312 [details] [diff] [review]
Don't show progress when entering about:reader
Comment 10 Lucas Rocha (:lucasr) 2012-09-07 12:05:25 PDT
Created attachment 659313 [details] [diff] [review]
Keep title and favicon when entering reader mode
Comment 11 Lucas Rocha (:lucasr) 2012-09-07 12:10:06 PDT
FYI: I'll be sending the last patch to implement the fade-in animation in about:reader as soon as I figure out a way to work around for what seems to be a graphics/layout bug in Gecko. For now, these patches are self-contained and can be reviewed independently.
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2012-09-07 21:53:13 PDT
Comment on attachment 659313 [details] [diff] [review]
Keep title and favicon when entering reader mode

More thinking required. I am not in love with having these checks in so many places. Ripe for maintenance issues or breakage.
Comment 13 Daniel Lombraña González 2012-09-10 00:38:49 PDT
Hi again,

Lucas, the graphics/layout issue is about a corrupted view in the phone? With the latest available build I have had sometimes an issue where instead of the web page I see broken pixels, like the GPU is not rendering well. Is this the issue you are describing? If not, I'll try to take a screenshot next time and create a new bug about it.

Cheers,

Daniel
Comment 14 Lucas Rocha (:lucasr) 2012-09-10 04:02:41 PDT
(In reply to Daniel Lombraña González from comment #13)
> Hi again,
> 
> Lucas, the graphics/layout issue is about a corrupted view in the phone?
> With the latest available build I have had sometimes an issue where instead
> of the web page I see broken pixels, like the GPU is not rendering well. Is
> this the issue you are describing? If not, I'll try to take a screenshot
> next time and create a new bug about it.

Looks like an unrelated issue. Please file a separate bug, thanks!
Comment 15 Lucas Rocha (:lucasr) 2012-09-10 07:04:34 PDT
Created attachment 659700 [details] [diff] [review]
Keep title and favicon when entering reader mode

Here's a simpler and better approach. The whole state is handled inside Tab. The check in BrowserToolbar's setTitle is inevitable as we sometimes shortcut title changes directly in the toolbar. It's OK to have this check there as the code is now grep-friendly (isEnteringReaderMode public API in Tab) and we have other checks for about: pages there already.
Comment 16 Mark Finkle (:mfinkle) (use needinfo?) 2012-09-10 07:59:35 PDT
Comment on attachment 659700 [details] [diff] [review]
Keep title and favicon when entering reader mode

Much nicer. Thanks.
Comment 17 Lucas Rocha (:lucasr) 2012-09-11 08:03:20 PDT
Created attachment 660077 [details] [diff] [review]
Fade Reader content once article is loaded
Comment 18 Lucas Rocha (:lucasr) 2012-09-11 08:03:39 PDT
Created attachment 660078 [details] [diff] [review]
Fix misc aboutReader.css warnings
Comment 19 Lucas Rocha (:lucasr) 2012-09-11 08:10:59 PDT
(In reply to Lucas Rocha (:lucasr) from comment #17)
> Created attachment 660077 [details] [diff] [review]
> Fade Reader content once article is loaded

Sorry, patch has wrong commit message. This patch is just about improving the way we handle progress and error messages in the Reader UI. I'll work on the fade in animation in bug 790253.
Comment 22 Lucas Rocha (:lucasr) 2012-09-12 03:18:44 PDT
Comment on attachment 659310 [details] [diff] [review]
Use ReaderModeUtils to create about:reader urls

[Approval Request Comment]
User impact if declined: These patches together greatly improve the experience of entering reader mode. Without them, the user will see some distracting flickering on the URL bar after clicking on the reader mode button. It also improves the perceived performance of the switch.
Testing completed (on m-c, etc.): Landed on m-c, no issues found.
Risk to taking this patch (and alternatives if risky): Fairly low, only applies to about:reader pages and it shouldn't affect anything else.
String or UUID changes made by this patch: None.
Comment 23 Lucas Rocha (:lucasr) 2012-09-12 03:18:50 PDT
Comment on attachment 659312 [details] [diff] [review]
Don't show progress when entering about:reader

[Approval Request Comment]
User impact if declined: These patches together greatly improve the experience of entering reader mode. Without them, the user will see some distracting flickering on the URL bar after clicking on the reader mode button. It also improves the perceived performance of the switch.
Testing completed (on m-c, etc.): Landed on m-c, no issues found.
Risk to taking this patch (and alternatives if risky): Fairly low, only applies to about:reader pages and it shouldn't affect anything else.
String or UUID changes made by this patch: None.
Comment 24 Lucas Rocha (:lucasr) 2012-09-12 03:19:00 PDT
Comment on attachment 659700 [details] [diff] [review]
Keep title and favicon when entering reader mode

[Approval Request Comment]
User impact if declined: These patches together greatly improve the experience of entering reader mode. Without them, the user will see some distracting flickering on the URL bar after clicking on the reader mode button. It also improves the perceived performance of the switch.
Testing completed (on m-c, etc.): Landed on m-c, no issues found.
Risk to taking this patch (and alternatives if risky): Fairly low, only applies to about:reader pages and it shouldn't affect anything else.
String or UUID changes made by this patch: None.
Comment 25 Lucas Rocha (:lucasr) 2012-09-12 03:19:07 PDT
Comment on attachment 660077 [details] [diff] [review]
Fade Reader content once article is loaded

[Approval Request Comment]
User impact if declined: These patches together greatly improve the experience of entering reader mode. Without them, the user will see some distracting flickering on the URL bar after clicking on the reader mode button. It also improves the perceived performance of the switch.
Testing completed (on m-c, etc.): Landed on m-c, no issues found.
Risk to taking this patch (and alternatives if risky): Fairly low, only applies to about:reader pages and it shouldn't affect anything else.
String or UUID changes made by this patch: None.
Comment 26 Lucas Rocha (:lucasr) 2012-09-12 03:19:44 PDT
Comment on attachment 660078 [details] [diff] [review]
Fix misc aboutReader.css warnings

[Approval Request Comment]
User impact if declined: These patches together greatly improve the experience of entering reader mode. Without them, the user will see some distracting flickering on the URL bar after clicking on the reader mode button. It also improves the perceived performance of the switch.
Testing completed (on m-c, etc.): Landed on m-c, no issues found.
Risk to taking this patch (and alternatives if risky): Fairly low, only applies to about:reader pages and it shouldn't affect anything else.
String or UUID changes made by this patch: None.
Comment 27 Alex Keybl [:akeybl] 2012-09-12 15:15:21 PDT
Comment on attachment 659310 [details] [diff] [review]
Use ReaderModeUtils to create about:reader urls

[Triage Comment]
Approving for Aurora to get extra testing, but we can wait a bit to take this fix on Beta (go to build is Tuesday).
Comment 28 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-17 15:14:19 PDT
Comment on attachment 659310 [details] [diff] [review]
Use ReaderModeUtils to create about:reader urls

Ok, let's get this in before tomorrow's beta go to build.
Comment 29 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-17 15:16:24 PDT
Comment on attachment 659310 [details] [diff] [review]
Use ReaderModeUtils to create about:reader urls

Just realized that these patches did not get landed to Aurora, removing the beta approvals I started to do.  We wanted to see how this would go on Aurora before approving for Beta to ensure no unnecessary risk.  Please land this to Aurora right away otherwise we're much less likely to accept it on a later Beta with no Aurora testing window.
Comment 31 Alex Keybl [:akeybl] 2012-09-18 12:05:13 PDT
Comment on attachment 659310 [details] [diff] [review]
Use ReaderModeUtils to create about:reader urls

This landed on Aurora too late to evaluate for Beta 4, and we don't take major forward change like this in Beta 5 or Beta 6. Risk to the new feature is enough to put this off till FF17.

Note You need to log in before you can comment on or make changes to this bug.