Closed Bug 1123910 Opened 5 years ago Closed 5 years ago

"about:reader" URL in location bar isn't user-friendly

Categories

(Firefox :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 38
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- fixed

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file, 1 obsolete file)

This is the desktop version of bug 1123904.

Currently, we show "about:reader?url=..." in the location bar when we load a page in reader mode, since technically, we are loading a special about: page and filling it with article content.

This looks kinda ugly, and perhaps there's something more useful we can show. However, implementation some special-case location bar logic for about:reader could be a pain, and maybe showing something different would be even more confusing, so long as we actually loaded a different page.

mmaslaney, have you thought about what we should show in the location bar when the user is on a reader mode page? I think some of your mock-ups just showed the page URL, but that might be confusing if the user wants to actually navigate back to that real URL (it might not mesh with their mental model of the location bar).

Maybe the real fix here would be to load the reader-ized content in an overlay, rather than loading a different page in the tab, but that would likely require a non-trivial amount of engineering work.
Flags: needinfo?(mmaslaney)
I think it would be nice to keep the URL unchanged, and have the button in the location bar be the toggle between what view gets shown. Maybe this is saying that the button is too far removed/hidden for the desktop UI?
I recently landed a bunch of changes improving how we parse and display moz-action: URIs in the URLBar - that machinery can be used here too. We now have a real URL (urlbar.value) and a display text (urlbar.textValue). That lets us have an internal URI that holds state, while displaying something nice in the UI (eg, a pretty URI and a badge to indicate the mode).
FTR, Sync/FxA has a similar problem - we often take the user to about:accounts?action=signIn&entryPoint=UITour&email=foo@bar.com&ugly=true etc.  comment 2 would help there too.
Margaret, how about we use "Title and domain".

For example:  
Designing for Easy Interaction · An A List Apart Article : http://alistapart.com/article/designing-for-easy-interaction
Flags: needinfo?(mmaslaney)
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #4)
> Margaret, how about we use "Title and domain".
> 
> For example:  
> Designing for Easy Interaction · An A List Apart Article :
> http://alistapart.com/article/designing-for-easy-interaction

Okay, I'll give that a try.

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #2)
> I recently landed a bunch of changes improving how we parse and display
> moz-action: URIs in the URLBar - that machinery can be used here too. We now
> have a real URL (urlbar.value) and a display text (urlbar.textValue). That
> lets us have an internal URI that holds state, while displaying something
> nice in the UI (eg, a pretty URI and a badge to indicate the mode).

Great news! Can you point me in the right direction for how to implement this for about:reader URLs? Should I be using a moz-action URI, or can I just set urlbar.textValue directly?

Here's the logic where we update the reader mode button and toggle reader mode, so I imagine logic for this change could go somewhere in here:
http://mxr.mozilla.org/mozilla-central/source/browser/modules/ReaderParent.jsm#91
Assignee: nobody → margaret.leibovic
Flags: needinfo?(bmcbride)
You should be able to avoid using a moz-action URI, but you can't set textValue directly as it'll be overwritten.
* Parse the URL in onBeforeValueSet (urlbarbindings.xml) - you may need to add the page title as a param in the URL.
* Return whatever value you want to be the display text.
* Set any additional attributes on the bindings to make it display a badge, and have that hooked up via CSS.
Flags: needinfo?(bmcbride)
Flags: firefox-backlog+
/r/3353 - Bug 1123910 - Display original article URL in urlbar when in reader mode (instead of about:reader)

Pull down this commit:

hg pull review -r 2345836fc5828ca27b1f586b2cf805f6d46fd888
Attachment #8559541 - Flags: review?(bmcbride)
This seems to work well! Thanks for the pointer.

This logic is the same as what we do in ReaderParent.jsm:
http://mxr.mozilla.org/mozilla-central/source/browser/modules/ReaderParent.jsm#125

Can I use ReaderParent.jsm in urlbarBindings.xml? I was unsure if that was okay, but a better solution would probably be to refactor that original function to be used in this new place as well.
https://reviewboard.mozilla.org/r/3353/#review2809

Tests?

::: browser/base/content/urlbarBindings.xml
(Diff revision 1)
> +          let searchParams = new URLSearchParams(aUrl.split("?")[1]);

Using .split() here doesn't seem safe.

::: browser/base/content/urlbarBindings.xml
(Diff revision 1)
> +      <method name="_parseReaderUrl">

Hmm... ReaderParent.jsm is already imported on startup, isn't it? Avoiding that would be my only reason to not use it here.
Attachment #8559541 - Flags: review?(bmcbride) → review-
https://reviewboard.mozilla.org/r/3353/#review3081

::: browser/base/content/test/general/browser_readerMode.js
(Diff revision 2)
> +  is(gURLBar.inputField.value, url.substring("http://".length), "gURLBar is displaying original article URL");

Use gURLbar.textValue to get the display value.

gURLBar.inputField.value is now just an implementation detail, only useful to use directly to mimic text being typed in.
Comment on attachment 8559541 [details]
MozReview Request: bz://1123910/margaret

https://reviewboard.mozilla.org/r/3351/#review3135

*sigh* Reviewboard.
Attachment #8559541 - Flags: review+
See Also: → 1134363
https://hg.mozilla.org/mozilla-central/rev/89a1843b7733
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Iteration: --- → 38.3 - 23 Feb
Flags: qe-verify?
This seems to have an automated test, so setting as qe-verify-.
Flags: qe-verify? → qe-verify-
Attachment #8559541 - Attachment is obsolete: true
Attachment #8619182 - Flags: review+
You need to log in before you can comment on or make changes to this bug.