Closed Bug 1482425 Opened Last year Closed Last year

Shift+PageDown/Shift+PageUp don't work in email composer if there is no vertical scrollbar

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P2)

60 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 - wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- fixed

People

(Reporter: andysem, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(3 files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180704192850

Steps to reproduce:

1. Select an email and press Reply. Composer window opens with the email text (in plain text mode) marked as quotation.
2. Navigate the cursor somewhere in the middle of the quotation.
3. Press Shift+PageDown or Shift+PageUp.


Actual results:

Nothing happens.


Expected results:

The cursor should move down or up, selecting the text between the previous and new positions. This works in Thunderbird 52.9.1. I often use these key combinations to quickly select and delete large portions of an email I'm replying to.
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
Version: 52 Branch → 60
I can add that Shift+PageDown/Shift+PageUp do work when the Composer window has a vertical scroll bar. The key combinations stop working if there is no scroll bar.
Masayuki-san, anything in the editor that changed here?
Flags: needinfo?(masayuki)
I have no idea. I can reproduce this bug with contenteditable element in Firefox. Needs regression range.
https://jsfiddle.net/d_toybox/yptn1zbs/1/
Status: UNCONFIRMED → NEW
Component: Message Compose Window → Keyboard: Navigation
Ever confirmed: true
Flags: needinfo?(masayuki)
Keywords: regression
OS: Linux → All
Product: Thunderbird → Core
Hardware: x86_64 → All
Summary: Shift+PageDown/Shift+PageUp don't work in email composer → Shift+PageDown/Shift+PageUp don't work in email composer if there is no vertical scrollbar
Version: 60 → 60 Branch
Alice, can you find where this regression occurred, either in TB or FF. Thanks in advance.
Flags: needinfo?(alice0775)
Blocks: 1369072
Thanks Alice. Sorry, I forgot to say "can you *please* ... ", but I'll say Thank You again :-) - Much appreciated.
Really thank you Alice-san!

That does make sense. The fix must correct the behavior for the method name. However, the method name is wrong or the caller uses the method with different purpose, that must be PresShell::PageMove().
https://searchfox.org/mozilla-central/rev/4b6737ba4008b71182cc2a12f507c82adf2b4e70/layout/base/PresShell.cpp#2312,2315
That needs a nearest scrollable frame rather than frame *to* scroll. So, perhaps, we need additional API to retrieve it and make PageMove() use it.
Priority: -- → P2
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
PresShell::PageMove() climbs up to parent document when there is no scrollable
parent in current document.  However, if aExtend is true, it should expand
Selection in the document itself.  Therefore, it needs different rules to
look for container of expanding Selection from scrollable element to scroll.

Additionally, old rules (i.e., before the fix of bug 1369072 which caused
this regression) were also buggy.  It used parent scrollable element or
root scrollable element simply.  Therefore, if found scrollable element is
ancestor of selection limiter, it didn't work as expected.

This patch creates nsFrameSelection::GetFrameToPageSelect() to retrieve
per-page selection container element with the following rules:
- look for a scrollable element in selection limiter.
- if there is no scrollable element, use selection limiter.
- if there is no selection limiter, use the root frame.

So, nsFrameSelection::CommonPageMove() should take nsIFrame rather than
nsIScrollableFrame since container of per-page selection may be used in
non-scrollable contenteditable element.  If it's called with non-scrollable
frame, it needs to compute the expanding range with the frame size.
FYI: On macOS, Shift+PageDown/PageUp without editor does not select the content because cmd_selectPageUp/cmd_selectPageDown are not mapped here: https://searchfox.org/mozilla-central/source/dom/xbl/builtin/mac/platformHTMLBindings.xml#33-54

If there is an editor, perhaps, EditorEventListener::KeyPress() resolves native command here: https://searchfox.org/mozilla-central/rev/3d989d097fa35afe19e814c6d5bc2f2bf10867e1/editor/libeditor/EditorEventListener.cpp#636
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/02a07fe87808
PresShell::PageMove() should use different rules to look for a container element for aExtend value r=smaug
https://hg.mozilla.org/mozilla-central/rev/02a07fe87808
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Masayuki-san, does that patch apply to m-esr60, then I can take it to TB's release branch. We had various complaints.
Duplicate of this bug: 1500678
Attached patch Patch for ESR60Splinter Review
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is regression of bug 1369072.

User impact if declined: This bug does not allow users to select contents if nearest editing host (e.g., <body> of designMode document or <foo contenteditable>) is not scrollable.

Fix Landed on Version: 64

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This changes the path to handle per-page selection and the cases are tested by the new automated test.

String or UUID changes made by this patch: none
Flags: needinfo?(masayuki)
Attachment #9019324 - Flags: approval-mozilla-esr60?
And the patch includes the random orange (bug 1499996), and this may break UX even on Firefox for desktop if composing email is not scrollable. So, this bug may be really annoying for some users who usually use Shift + PageUp/PageDown.
Thanks for the additional comments on severity. Setting the status back to affected for 60.4 consideration. TB may want to graft it to their verbranch for now if they're planning a 60.3.x release with it.
Flags: in-testsuite+
Works fine in TB 60.3. Thanks for the backport, Masayuki-san.
Unless we see some definite ESR user impact, I'd like to defer this to the next major ESR release. 
It may be risky and we try to keep change in ESR to major security or stability fixes after the first couple of versions. Good to see that it is now fixed for TB.
Attachment #9019324 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60-
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.