778489 - Implement nice transition to Reader Mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect, P1)

Description

[Peter Retzer (:pretzer)]

Attachment: 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]

I confirm this bug.

Comment 2

[Lucas Rocha (:lucasr)]

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)]

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)]

Ian, Madhava, feedback is welcome.

Comment 5

[Daniel Lombraña González]

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)]

(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]

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)]

Attachment: Use ReaderModeUtils to create about:reader urls

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: Don't show progress when entering about:reader

Comment 10

[Lucas Rocha (:lucasr)]

Attachment: Keep title and favicon when entering reader mode

Comment 11

[Lucas Rocha (:lucasr)]

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?)]

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]

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)]

(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)]

Attachment: 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?)]

Comment on attachment 659700 [details] [diff] [review]
Keep title and favicon when entering reader mode

Much nicer. Thanks.

Comment 17

[Lucas Rocha (:lucasr)]

Attachment: Fade Reader content once article is loaded

Comment 18

[Lucas Rocha (:lucasr)]

Attachment: Fix misc aboutReader.css warnings

Comment 19

[Lucas Rocha (:lucasr)]

(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 20

[Lucas Rocha (:lucasr)]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Lucas Rocha (:lucasr)]

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)]

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)]

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)]

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)]

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]]

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]

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]

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 30

[Lucas Rocha (:lucasr)]

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 31

[Alex Keybl [:akeybl]]

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.