Implement nice transition to Reader Mode

VERIFIED FIXED in Firefox 17

Status

()

Firefox for Android
Reader View
P1
normal
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: pretzer, Unassigned)

Tracking

Trunk
Firefox 18
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox17 fixed, firefox18 verified)

Details

(Whiteboard: ui-responsiveness)

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
I confirm this bug.
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.
Priority: -- → P2
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.
Summary: Reader Mode: Awesomebar shows ugly URL while loading the article in reader mode → Implement nice transition to Reader Mode
Ian, Madhava, feedback is welcome.
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
(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 :-)

Updated

5 years ago
Whiteboard: ui-responsiveness
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)
Created attachment 659310 [details] [diff] [review]
Use ReaderModeUtils to create about:reader urls
Attachment #659310 - Flags: review?(mark.finkle)
Created attachment 659312 [details] [diff] [review]
Don't show progress when entering about:reader
Attachment #659312 - Flags: review?(mark.finkle)
Created attachment 659313 [details] [diff] [review]
Keep title and favicon when entering reader mode
Attachment #659313 - Flags: review?(mark.finkle)
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.
Attachment #659310 - Flags: review?(mark.finkle) → review+
Attachment #659312 - Flags: review?(mark.finkle) → review+
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.
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
(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!
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.
Attachment #659313 - Attachment is obsolete: true
Attachment #659313 - Flags: review?(mark.finkle)
Attachment #659700 - Flags: review?(mark.finkle)
Comment on attachment 659700 [details] [diff] [review]
Keep title and favicon when entering reader mode

Much nicer. Thanks.
Attachment #659700 - Flags: review?(mark.finkle) → review+

Updated

5 years ago
Priority: P2 → P1
Created attachment 660077 [details] [diff] [review]
Fade Reader content once article is loaded
Attachment #660077 - Flags: review?(mark.finkle)
Created attachment 660078 [details] [diff] [review]
Fix misc aboutReader.css warnings
Attachment #660078 - Flags: review?(mark.finkle)
(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.
Attachment #660077 - Flags: review?(mark.finkle) → review+
Attachment #660078 - Flags: review?(mark.finkle) → review+
Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff9853a03d8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/110ae125057f
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fb1bb770b63
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e35e2c4b0d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe4be63821b
https://hg.mozilla.org/mozilla-central/rev/ff9853a03d8e
https://hg.mozilla.org/mozilla-central/rev/110ae125057f
https://hg.mozilla.org/mozilla-central/rev/0fb1bb770b63
https://hg.mozilla.org/mozilla-central/rev/9e35e2c4b0d2
https://hg.mozilla.org/mozilla-central/rev/dfe4be63821b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
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.
Attachment #659310 - Flags: approval-mozilla-beta?
Attachment #659310 - Flags: approval-mozilla-aurora?
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.
Attachment #659312 - Flags: approval-mozilla-beta?
Attachment #659312 - Flags: approval-mozilla-aurora?
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.
Attachment #659700 - Flags: approval-mozilla-beta?
Attachment #659700 - Flags: approval-mozilla-aurora?
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.
Attachment #660077 - Flags: approval-mozilla-beta?
Attachment #660077 - Flags: approval-mozilla-aurora?
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.
Attachment #660078 - Flags: approval-mozilla-beta?
Attachment #660078 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Status: RESOLVED → VERIFIED
status-firefox18: --- → verified
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).
Attachment #659310 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #659312 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #659700 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #660077 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #660078 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #659310 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #659312 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #659312 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
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.
Attachment #659310 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ba05e0e026d8
https://hg.mozilla.org/releases/mozilla-aurora/rev/dc82aa64358c
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0a8a8dda952
https://hg.mozilla.org/releases/mozilla-aurora/rev/187904a0cbfd
https://hg.mozilla.org/releases/mozilla-aurora/rev/da83e72532da
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.
Attachment #659310 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Updated

5 years ago
Attachment #659312 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Updated

5 years ago
Attachment #659700 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Updated

5 years ago
Attachment #660077 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Updated

5 years ago
Attachment #660078 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Updated

5 years ago
status-firefox17: --- → fixed
You need to log in before you can comment on or make changes to this bug.