1144749 - Use accel-(opt/alt)-r as the reader mode keyboard shortcut

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect, P3)

Description

[lriutzel]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150314004030

Steps to reproduce:

Try to find a keyboard shortcut for the new firefox reader mode.


Actual results:

No known keyboard shortcut exists.


Expected results:

There should be some keyboard combination that will bring up reader mode. Maybe configurable?
Keyboard shortcut suggestion Ctrl+E?

Wonderful work btw.

Comment 1

[Bryan Clark (DevTools PM) [:clarkbw]]

note that reading list ( bug 1138734 ) may be changing its shortcut.  should keep this in mind while doing so.

Comment 2

[felix-mozilla]

I propose F9.

Comment 4

[Jiehong]

I think that right now the reader button appears or not, and the user simply does not understand why it does not show sometimes.
This is a first problem: the user should know when reader mode is available or not, and especially why.

If a shortcut is added, we will be able to force to activate reader on pages that may not render properly, nor were supposed to be handled by reader.
If we do, then reader must explain why a given page is not being rendered properly when activated by the user.

Comment 5

[Lu Wei]

I propose let the user decide whether or not to enter reader mode. If he tries to activate reader mode on an improper page, just render it anyway, and the user will notice anything irregular.

Comment 6

[Jiehong]

If so, the user may think it's a broken functionality, leading to less usage of this functionality and/or more bug reports.

Perhaps Firefox should try to warn the user that the current page may render improperly, and offers to report that page.

It's like if a user buys a pressure cooker that is unreliable with no marks. The user may use it, but then perhaps the pressure will be too high, and some pressure cookers will explode. Users will certainly learn to use it, or simply not use it because it's not reliable at all.
That is a serious design issue.

Comment 7

[Lu Wei]

I second that. Rendering the page with a warning at the top seems suitable.

Comment 8

[Gilbert]

Or just put a message that tell the user that the page is not renderable, then Firefox will not render it

Comment 9

[Lu Wei]

I will appreciate if a "render it anyway" option be provided.

Comment 10

[Mario Grgic]

In Safari the reader button always appears and it just gets enabled or disabled if the reader view can be rendered. 

Also, the keyboard shortcut does not do anything if the reader view button is disabled, which is perfectly acceptable.

Comment 11

[ackled]

I am in too. Requesting hotkey for Reading mode.

Comment 12

[Luke]

I want a keyboard shortcut as well. Maybe Alt+R?

Comment 13

[Kudos]

I add my vote. The Reading mode feature is one of the most useful yet. A shortcut is necessary when breezing through a number of tabs/articles at a time. Preferably something customizable in the settings, else, something standard across PC/Mac.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Now that bug 1184005 and bug 1138734 have been fixed, we can use Cmd+Option+R to open Reader Mode. Mark, would you like to mentor this bug?

Comment 15

[Mark Hammond [:markh] [:mhammond]]

(In reply to (spotty availability 8/7 and 8/10) Jared Wein [:jaws] (please needinfo? me) from comment #14)
> Now that bug 1184005 and bug 1138734 have been fixed, we can use
> Cmd+Option+R to open Reader Mode. Mark, would you like to mentor this bug?

Sure!

Comment 16

[Underpass]

I think this can also meet the needs of left-handed people (like me): it is very annoying to make the mouse pointer travel across the screen to turn the Reader Mode on.
A keyboard shortcut is therefore very well accepted.

Comment 17

[Gilbert]

Suggestion: able to identify if the text is code. For example you reading and article about programing, then there is a sample code included then those code sample will have different font or something.

Comment 18

[Alex Czar]

Went over the comments, there are some great suggestions, but let's keep this request simple and create other tickets for those suggestions. Here let's discuss the need for the shortcut, nothing else.
To avoid pressure on the developers, let's start simple: 

        We need a shortcut for the 'Enter reader view' action first.
****----____                                                    ____----****

Then we can consider improvements to the feature itself.

Comment 19

[Pesach]

I agree entirely with the previous comment.

Comment 21

[Maurya Talisetti]

Does this qualify as a good-next-bug?

Comment 22

[Mark Hammond [:markh] [:mhammond]]

(In reply to Maurya Talisetti from comment #21)
> Does this qualify as a good-next-bug?

Yeah, I think so. The task would be to modify browser-sets.inc to add a new key definition much like the ones already there. Instead of "command" it would probably need an |oncommand| handler. The trickiest but I can forsee is knowing what to do if readermode isn't enabled for the page - presumably it would just ignore the request (I don't think this bug is the correct place to argue it should be force-enabled). One thought is that |toggleReaderMode()| in ReaderParent.jsm should check if reader mode can actually be entered and early-return if it can't (currently it assumes it can) - in which case the oncommand handler could just call that.

Comment 24

[Julien Wajsberg [:julienw]]

I just want to add a +1: someone asked me about this during a conference last week.

Comment 25

[Eric Piel]

There should be a decision on which shortcut it should be. For information, Safari uses Command+Shift+R. As Ctrl+Shift+R is already in used, it's not possible to do a direct mapping.

So far the propositions in the comments have been:
* Ctrl+E
* Alt+R
* F9

None of them seem to conflict with any current shortcut. Personally, I'm fine with all of them. I just note that they are only very few shortcuts in Firefox following the pattern Alt+?, so Alt+R is maybe a bit less appropriate.

Comment 26

[heinrichmartin]

From the above list, I'd argue for F9 as it compares well to the full screen mode on F11. Both, F10 (toggle menu bar) and F12 (toggle developer tools) are used by other features, so F9 is the closest option.

Comment 27

[Alex C]

Current situation with shortcuts in Firefox on Windows:
* Ctrl+R - reload page
* Ctrl+Shift+R - reload page, force cache cleaning
* Ctrl+E - focus to search bar

Be frankly I would like to have 'R' in the shortcut as it will follow the '(R)eading' semantic. Though I have not seen shortcuts with just Alt modifier. Maybe Ctrl+Alt+R could be a good choice?

BTW: Does it mean that functionality is ready and we just need to select default shortcut? If so then I would like to propose to use any as anybody could use extensions like Customizable Shortcuts and modify it to any he would like. :)

Comment 28

[Sajay]

(In reply to heinrichmartin from comment #26)
> From the above list, I'd argue for F9 as it compares well to the full screen
> mode on F11. Both, F10 (toggle menu bar) and F12 (toggle developer tools)
> are used by other features, so F9 is the closest option.

I vote for F9 too. Suits well as explained by heinrichmartin.

Comment 29

[Mario Grgic]

Note that F9 is not so convenient on a Mac OS X, since it is really fn + F9. If we are shooting for single key shortcut that it should be platform specific. Otherwise, it should be user customizable if at all possible.

Comment 30

[Akshay S Dinesh]

I'll vote against F9 in favour of a shortcut that doesn't involve function keys for the following reasons:

1) Like Mario said, F9 is fn+F9 in many keyboards these days (not just Mac)
2) I can't touch type Function keys (and I believe there's not a lot of tutorials for touch typing that goes above the number row and/or that not a lot of people learn/practise touch typing function keys). Now, I read a lot while lying down on my bed. In this particular position, I can't look at my keyboard. This makes it difficult for me to type the function key without bending and raising my head.

Comment 31

[disciple1211]

While I could get used to using the f9 key as a shortcut, I think that a key such as shift-alt-R might be a bit more intuitive and it is similar to the shortcut used in Safari to activate its own reader view. Ctrl-alt-R might be OK but Windows often uses the ctrl-alt combo as default modifiers for user-defined shortcut keys for launching applications. Many visually impaired users make use of assigning shortcut keys to applications and it is partially for this group of users that I'd personally like to see this hotkey assigned.

Comment 32

[Crick]

For the sake of OSX users, downvote for function keys (e.g. F9) as function keys are assigned to OSX functions (i.e. F9 is skip/next media.  

+1 for CTRL-ALT-R

Comment 33

[Dmitri Minaev]

Ctrl-E is easier when your fingers are on the home row. OTOH, when did Mozilla care about home row :)

Comment 34

[Sajay]

Lets analyze the options proposed till now

F9 - not easy to touch type, not convenient in Mac and new keyboards

Ctrl + Alt + [X] - most of such shortcuts does something out of the application in focus. Launches applications in Windows, manipulates workspaces in gnome... so it is a bit counter intuitive

Other three-key shortcuts - I would not recommend having a three-key shortcut as it would be equivalent to Alt + V + R, the current menu based shortcut for reading view

Alt + [X] - As alt based shortcuts are not so common in firefox, I would not suggest using it.

Why don't we use Ctrl + E?
- It no longer takes you to the address bar (replaced with Ctrl + L)
- Though "E" is not the first letter in "Reading" it is the second letter. Further "E" is the first letter in ebook, epub - both of which reminds me of the reading view. 
- As Dmitri Minaev pointed put, it is easier to use Ctrl + E when the fingers are on the home row. users who have addons like VimFx will prefer this. As it can be touch typed it is easy for visually impaired users too.
- As for right-handed users who use the mouse for browsing, it is easy to press Ctrl + E using the left hand. The right hand can stay on the mouse.

So +1 for Crlt + E

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

Ctrl+E takes the user to the search box. If the search box is in the Firefox menu, then it opens the menu. If the search box is removed from the UI, it takes the user to about:home.

Comment 36

[Jiehong]

Somehow, Alt + [X] is used for some shortcuts (Alt-D, Alt-digit, Alt-enter, Alt-F4), and having the "R" of reader would make it very suitable. Because it's not used that much doesn't mean we should avoid it.

Ctrl + E: maybe it's just me, but it means "end of line" in my fingers muscles. Plus, the accessibility of the E and the R are the same. So not a good pick for me.

F9: not convenient on some keyboards? Well, same as F5, F11, F10. Anyway, it's a one-time press, not something you do all the time.

+1 for Alt + R and F9

Comment 38

[Horatiu Lazu]

I would be interested on working on this bug (first bug). Is that okay?

Comment 39

[:Gijs (he/him)]

(In reply to Horatiu from comment #38)
> I would be interested on working on this bug (first bug). Is that okay?

Yes, welcome! I'm assuming you've built Firefox already? If not, that's fine, but you'll need to do so in order to fix this bug. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build and https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Artifact_builds for documents that might be helpful (or ask on IRC: https://wiki.mozilla.org/IRC ).

You'll need to create a localization entity called "enterReaderMode.key", with the value "R", in http://searchfox.org/mozilla-central/rev/496904277ce0143bc1a952f2eb2c7e6a81aa3d4d/browser/locales/en-US/chrome/browser/browser.dtd#113 .

Then you'll need to create a <key> element right before this one:

https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/browser/base/content/browser-sets.inc#288

that uses that new entity and has modifiers "accel alt" and invokes the command "View:ReaderView". You'll also need to give the <key> element an ID, and mark it as disabled="true" to start with.

Finally, in this code: http://searchfox.org/mozilla-central/rev/496904277ce0143bc1a952f2eb2c7e6a81aa3d4d/browser/modules/ReaderParent.jsm#99-107 you'll need to add a line that add/removes that "disabled" attribute based on browser.isArticle (see the line that toggles the command being hidden or not).

If you have questions, feel free to ask in here or ping me on IRC ( https://wiki.mozilla.org/IRC ).

Comment 40

[Horatiu Lazu]

(In reply to :Gijs Kruitbosch (Gone July 28 - Aug 11) from comment #39)
> (In reply to Horatiu from comment #38)
> > I would be interested on working on this bug (first bug). Is that okay?
> 
> Yes, welcome! I'm assuming you've built Firefox already? If not, that's
> fine, but you'll need to do so in order to fix this bug.
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Build_Instructions/Simple_Firefox_build and
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Build_Instructions/Artifact_builds for documents that might be helpful (or
> ask on IRC: https://wiki.mozilla.org/IRC ).
> 
> You'll need to create a localization entity called "enterReaderMode.key",
> with the value "R", in
> http://searchfox.org/mozilla-central/rev/
> 496904277ce0143bc1a952f2eb2c7e6a81aa3d4d/browser/locales/en-US/chrome/
> browser/browser.dtd#113 .
> 
> Then you'll need to create a <key> element right before this one:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/browser/base/content/browser-sets.
> inc#288
> 
> that uses that new entity and has modifiers "accel alt" and invokes the
> command "View:ReaderView". You'll also need to give the <key> element an ID,
> and mark it as disabled="true" to start with.
> 
> Finally, in this code:
> http://searchfox.org/mozilla-central/rev/
> 496904277ce0143bc1a952f2eb2c7e6a81aa3d4d/browser/modules/ReaderParent.jsm#99-
> 107 you'll need to add a line that add/removes that "disabled" attribute
> based on browser.isArticle (see the line that toggles the command being
> hidden or not).
> 
> If you have questions, feel free to ask in here or ping me on IRC (
> https://wiki.mozilla.org/IRC ).

Thank you very much! I'm really excited. I haven't finished building FireFox yet but I will be working this week on this bug. I'll let you know on my progress! Thanks again.

Comment 41

[Horatiu Lazu]

Hello Gijs. I got the build working for FireFox, and I followed your instructions. I built it, but it doesn't seem to work. Do you see anything wrong with my code? These are the lines I created from your 3 steps (in order):

(Line #144, Line #288, Line #108)
-----
<!ENTITY enterReaderMode.key "R">

<key id="enterReaderMode" key="&enterReaderMode.key;" command="View:ReaderView" modifiers="accel alt" disabled="true"/>

command.setAttribute("disabled", !browser.isArticle);

Thanks!

Comment 42

[Mark Hammond [:markh] [:mhammond]]

Hi Horatiu,
  Gijs is on holiday for a couple of weeks, but I'm happy to help in the meantime. It looks like the modifiers attribute should be "accel,alt" rather than "accel alt". Could you try that?

If you still have problems, please upload a patch with your current work (it will be much easier to both see exactly what changes you made, and to test the changes locally) and a clear description of what problems you are having (in this case I suspect the issue us "I pressed ctrl+alt+R and nothing happened", but it's worth being clear)

Comment 43

[Horatiu Lazu]

Attachment: shortcut.diff

This is my attempt for the bug #1144749. When I try to call the shortcut (Cmd+Alt+R) it does not seem to do anything. Also, let me know if the patch file was made correctly, it took me all day to get it to work :/

Comment 44

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8775821 [details] [diff] [review]
shortcut.diff

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

(In reply to Horatiu from comment #43)
> Created attachment 8775821 [details] [diff] [review]
> shortcut.diff
> 
> This is my attempt for the bug #1144749. When I try to call the shortcut
> (Cmd+Alt+R) it does not seem to do anything.

The patch is very close to working :)

The main problem is that in comment 39, the comment:

> you'll need to add a line that add/removes that "disabled" attribute based on browser.isArticle
> (see the line that toggles the command being hidden or not).

Means that you should toggle the disabled attribute on the new key element, not the existing command. So in this patch the key itself remains disabled forever, so never does anything.

With the changes I outline below it should work correctly - the first press will enter reader-mode, and a second press will exit reader-mode.

> Also, let me know if the patch
> file was made correctly, it took me all day to get it to work :/

That patch file was made correctly, however it has unrelated changes to .hgignore that you should revert before uploading a new version.

Also, the "review" and "feedback" flags are tricky to understand. In general, when uploading a patch you should set either the "feedback" or "review" flags to "?" and enter the person you are requesting review or feedback on. When the patch is reviewed, the reviewer will change it to either "+" or "-" - generally, you should not set either the "+" or "-" flags yourself.

The "review" flag is used when you believe the patch is ready to land, whereas the "feedback" flag is when it's not quite ready yet, but you need some help or advice. For this patch, "feedback" would have been the most appropriate.

So if the next version of your patch works and you think it is ready, request just "r?" from me. If you still have questions about it or aren't quite sure if it is ready, request just "f?" from me.

Thanks for your efforts here!

::: browser/base/content/browser-sets.inc
@@ +284,5 @@
>      <key id="key_fullScreen" key="&fullScreenCmd.macCommandKey;" command="View:FullScreen" modifiers="accel,control"/>
>      <key id="key_fullScreen_old" key="&fullScreenCmd.macCommandKey;" command="View:FullScreen" modifiers="accel,shift"/>
>      <key keycode="VK_F11" command="View:FullScreen"/>
>  #endif
> +    <key id="enterReaderMode" key="&enterReaderMode.key;" command="View:ReaderView" modifiers="accel,alt" disabled="true"/>

I think you should use id="toggleReaderMode" as that is what the key will actually be doing.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +110,5 @@
>  <!ENTITY fullScreenCmd.accesskey "F">
>  <!ENTITY fullScreenCmd.macCommandKey "f">
>  <!ENTITY showAllTabsCmd.label "Show All Tabs">
>  <!ENTITY showAllTabsCmd.accesskey "A">
> +<!ENTITY enterReaderMode.key "R">

and change this to match the id (ie, toggleReaderMode.key)

::: browser/modules/ReaderParent.jsm
@@ +91,1 @@
>      if (browser.currentURI.spec.startsWith("about:reader")) {

just above this line, you need to get a reference to the new key element - so something like |let key = win.document.getElementById("toggleReaderMode");|

@@ +95,5 @@
>        button.setAttribute("tooltiptext", closeText);
>        command.setAttribute("label", closeText);
>        command.setAttribute("hidden", false);
>        command.setAttribute("accesskey", gStringBundle.GetStringFromName("readerView.close.accesskey"));
> +      command.setAttribute("disabled", true);

here and below, we want the "disabled" attribute on the key to exactly match the "hidden" attribute on the command - so this line would be |key.setAttribute("disabled", false)| - we are using false here as that is what the command element has as the hidden attribute. IOW, we want the key to be enabled here as the key will be used to *exit* reader mode in this case.

@@ +104,5 @@
>        button.setAttribute("tooltiptext", enterText);
>        command.setAttribute("label", enterText);
>        command.setAttribute("hidden", !browser.isArticle);
>        command.setAttribute("accesskey", gStringBundle.GetStringFromName("readerView.enter.accesskey"));
> +      command.setAttribute("disabled", !browser.isArticle);

the value here for disabled is correct (ie, it matches "hidden" on the command) but we still want to change the key, not the command - so it will read |key.setAttribute(...)| instead of |command.|

Comment 45

[Horatiu Lazu]

Attachment: addKeyboardShortcut.patch

Hi Mark, thank you very much for the VERY detailed instructions! The shortcut key (Cmd+Alt+R) works for me locally (toggles reader view), I'm very happy. I struggled quite a bit with Mercurial, but I hope I managed to get that down properly. Let me know if I have to change anything else, thanks!

Comment 46

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8776226 [details] [diff] [review]
addKeyboardShortcut.patch

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

That looks great - thanks, and congratulations!

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c2849a432eee
Add keyboard shortcut to open ReaderView. r=markh

Comment 48

[Horatiu Lazu]

Thank you, I'll go look for more bugs now :-)

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2849a432eee

Comment 50

[Sylvestre Ledru [:Sylvestre]]

We should relnotes that. not sure what the wording should be

Comment 51

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx50 (Aurora) release notes.

Comment 52

[Markus Zapke-Gründemann]

Works in Firefox Developer Edition on OS X. Thanks adding the shortcut!

Comment 53

[Carmen]

I have verified the fix on latest Nightly (2016-09-15) and latest Aurora (2016-09-15) using Mac OS X 10.12 Sierra Beta. The shortcut works fine.

Comment 54

[eyal gruss]

not working for me on firefox 50 (windows)

Comment 55

[Mark Hammond [:markh] [:mhammond]]

Works for me on 50 on Windows. Note that you must see the reader-mode icon in the URL bar, then pressing ctrl+alt+r toggles reader mode just like clicking that icon does.

Comment 56

[Stephan Henningsen]

Ctrl+Alt+R? Seriously? Why not throw in Shift now you're at it?  It's called Reader View for a reason; not let's-do-a-triple-combo-key-press-mode!

Please change to F9. It's not used and really easy to find, sitting there on the edge of the last four-block of F-keys.

If no sane default short cut key will be adopted, how do I at least change it to F9?

Comment 57

[john]

CTRL+ALT+R defeats the purpose of having a shortcut in the first place. Either add an entry in about:config to select another or change it to F9 key.

Comment 58

[Revertron]

But now the F9 starts reader mode AND opens my extension's pageAction panel.
https://addons.mozilla.org/firefox/addon/art-password/

Comment 59

[:Gijs (he/him)]

(In reply to reverse from comment #58)
> But now the F9 starts reader mode AND opens my extension's pageAction panel.
> https://addons.mozilla.org/firefox/addon/art-password/

We changed this on Windows for Firefox 60 and later in bug 1438308. I understand that that's annoying, but it wasn't reasonable to keep the ctrl-alt shortcut given general platform restrictions and concerns.

Comment 60

[john]

thanks for the useless "shortcut"