Closed Bug 1134446 Opened 5 years ago Closed 5 years ago

Automatically open the ReadingList sidebar the first time ReaderMode is used

Categories

(Toolkit :: Reader Mode, defect, P3)

defect
Points:
3

Tracking

()

VERIFIED FIXED
mozilla39
Iteration:
39.3 - 30 Mar
Tracking Status
firefox38 --- verified
firefox39 --- verified

People

(Reporter: Unfocused, Assigned: capella)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

To help introduce ReadingList, we want to automatically open the sidebar the first time ReaderMode is used (and ReadingList is enabled).

This should only happen the first time. It's not clear whether the sidebar should auto-close when ReaderMode is exited.
Flags: qe-verify+
Flags: firefox-backlog+
Michael: Thoughts on auto-closing the sidebar?

I guess one difficulty here if we auto-closed is we'd need to keep track of when a tab is closed, or a tab is switched to. Otherwise things would feel unpredictable. ie:
* open readermode
* sidebar auto-opens
* switch to another tab
* browser around for half an hour with the sidebar open
* switch to original readermode tab
* exit readermode
* sidebar suddenly closes by itself - wtf?!
Flags: needinfo?(mmaslaney)
Blocks: 1132074
Attached patch bug1134446.diff (obsolete) — Splinter Review
First hack. Doesn't address auto-close.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #8566369 - Flags: review?(bmcbride)
Attached patch bug1134446.diff (obsolete) — Splinter Review
Rebased here too.
Attachment #8566369 - Attachment is obsolete: true
Attachment #8566369 - Flags: review?(bmcbride)
Attachment #8566913 - Flags: review?(bmcbride)
Attached patch bug1134446.diff (obsolete) — Splinter Review
rebased, new test updated

:-) Sorry for bug-spam
Attachment #8566913 - Attachment is obsolete: true
Attachment #8566913 - Flags: review?(bmcbride)
Attachment #8567641 - Flags: review?(bmcbride)
Comment on attachment 8567641 [details] [diff] [review]
bug1134446.diff

Let's wait here until bug 1133489 finalizes the base test, so I don't have to rebase again.
Attachment #8567641 - Flags: review?(bmcbride)
My inclination is to leave it open, that way we communicate to the user that the Reading List is not dependent on the Reader View.
Flags: needinfo?(mmaslaney)
Attached patch bug1134446.diff (obsolete) — Splinter Review
Rebased, tested.
Attachment #8567641 - Attachment is obsolete: true
Attachment #8572062 - Flags: review?(bmcbride)
Priority: -- → P2
Priority: P2 → P3
No longer blocks: 1132074
Comment on attachment 8572062 [details] [diff] [review]
bug1134446.diff

Review of attachment 8572062 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-readinglist.js
@@ +184,5 @@
>        }
> +
> +      case "ReadingList:ShowIntro": {
> +        if (this.enabled && !Services.prefs.getBoolPref("reader.has_shown_readinglist")) {
> +          Services.prefs.setBoolPref("reader.has_shown_readinglist", true);

Doesn't seem right for this to be setting reader.* prefs. Especially given this code doesn't necessarily know this came from ReaderView. And also, this will eventually kick off a ReadingList oriented tour (see mockup linked to in bug 1131112). So how about "browser.readinglist.introShown"?

Also, nit: use |Preferences.get("pref-name", defaultValue)|, and avoid needing a default pref set in firefox.js

::: browser/base/content/test/general/browser_readerMode.js
@@ +46,5 @@
>    is(gURLBar.value, readerUrl, "gURLBar value is about:reader URL");
>    is(gURLBar.textValue, url.substring("http://".length), "gURLBar is displaying original article URL");
>  
> +  // Readinglist button should be present, and status should be automatically
> +  // "opened", as requested in bug bug1134446.

It's not safe to assume the relevant pref is set here - you don't know what other tests have done. Safest for the test to set it to a known state first.

Also: Don't need to reference bugs - that's what blame/annotate is for.
Attachment #8572062 - Flags: review?(bmcbride) → review-
(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #8)
> It's not safe to assume the relevant pref is set here - you don't know what
> other tests have done. 

Oh, in fact, this behaviour may mess up other tests - best to disable it by default in the test harness. Just need to add the pref to testing/profiles/prefs_general.js such that it won't automatically open, except for this test.
Attached patch bug1134446.diffSplinter Review
Attachment #8572062 - Attachment is obsolete: true
Attachment #8576445 - Flags: review?(bmcbride)
QA Contact: andrei.vaida
Depends on: 1136570
No longer depends on: 1132988
Comment on attachment 8576445 [details] [diff] [review]
bug1134446.diff

Review of attachment 8576445 [details] [diff] [review]:
-----------------------------------------------------------------

Bit-rotted ... back-burner-ing this for now.
Attachment #8576445 - Flags: review?(bmcbride)
Comment on attachment 8576445 [details] [diff] [review]
bug1134446.diff

Review of attachment 8576445 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I didn't get to this - I was off on sick leave for a bit.

r+ on the assumption that the bitrot doesn't mean you end up rewriting it ;)
Attachment #8576445 - Flags: review+
Glad you're back!

Bitrot caused some minor test/general/browser_readerMode.js rebase mainly. Passes locally and on TRY ...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=65f92c424f46

... so I'll carry over the r+.
https://hg.mozilla.org/mozilla-central/rev/8fd82fc12d25
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Iteration: --- → 39.3 - 30 Mar
Verified fixed on Aurora 39.0a2 (2015-04-02), using Ubuntu 14.04 (x64), Windows 7 (x64) and Mac OS X 10.9.5.
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1147484
Comment on attachment 8576445 [details] [diff] [review]
bug1134446.diff

Approval Request Comment
[Feature/regressing bug #]: needed for reading list uitour
[User impact if declined]: reading list uitour is somewhat hampered
[Describe test coverage new/current, TreeHerder]: automated testing, landed on m-c
[Risks and why]: none expected
[String/UUID change made/needed]: none
Attachment #8576445 - Flags: approval-mozilla-beta?
Attachment #8576445 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified fixed on Beta 38.0b2-build1 (20150406174117) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.
You need to log in before you can comment on or make changes to this bug.