1499430 - PgUp/PgDown doesn't move cursor in mail compose

Status: RESOLVED FIXED

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

Description

[thomas.hebinck]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:62.0) Gecko/20100101 Firefox/62.0

Steps to reproduce:

Open mail compose (plain text), have text less than the height of the window, press PageUp/PageDown keys.


Actual results:

Nothing.


Expected results:

The cursor should move to the top/bottom of the text (as before).
Exact version is: 60.2.1 (64-bit) on Ubuntu

Comment 2

[Jorg K (CEST = GMT+2)]

Oops, this is not a duplicate of bug 1482425 since that one is about Shift+PageUp/Down.

I've just tried, yes, in TB 52 on a document with no scrollbar, the cursor moves when PageUp/Down are used.

Alice, can you please find the regression.

Comment 6

[Alice0775 White]

Regression window(comm-central):
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=def874d2d27c8fa1734f885059d29362b3990032&tochange=4fc52379f051d1bfc91eda6cf789d2d2119e3526
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=50857982881ae7803ceb438fee90650a282f7f05&tochange=c71b01e993510268bab7d60154b2f80692fd507d

And the problem can be reproduced in Firefox57 and Nightly66.0a1 as well as in Thunderbird.

STR:

1. data:text/html,<div contentEditable="true" style="height:100px; border:1px solid black;"><p>foo bar baz</p><p>hoge huga piyo</p></div>
2. Put caret on 1st line
3. press PageUp/PageDown keys

Regression window(mozilla-central):
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=50857982881ae7803ceb438fee90650a282f7f05&tochange=dd3736e98e4e7558af2f202803bce36278a26c66

Suspect:
ef1641e40903 Masayuki Nakano — Bug 1369072 - part3: nsXBLPrototypeHandler::DispatchXBLCommand() should use controller of visible window r=smaug
1c3ac6cb53ea Masayuki Nakano — Bug 1369072 - part2: PresShell::HandleEvent() should retarget KeyboardEvent if focused document is invisible r=smaug
19f8a5d3da39 Masayuki Nakano — Bug 1369072 - part1: PresShell should climb up scrollable frames when there is no selection, no focused content and root scrollable frame isn't scrollable r=smaug
2e507cfef60d Masayuki Nakano — Bug 1369072 - part0: Add automated test for testing key event handlers to scroll parent document when an iframe element has or had focus r=smaug

Comment 7

[Wayne Mery (:wsmwk)]

reporter of bug 1518067 "Note that this does work once the body text gets longer than the window. Once the scrollbar appears, page up and page down start working normally."

Comment 8

[Jorg K (CEST = GMT+2)]

Thank you very much, Alice. So bug 1369072 regressed both bug 1482425 (Shift+PageUp/Down) as well as this bug here (PageUp/Down).

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sigh... Why we don't have this kind of tests...

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1921db1b7956d615cf4824f12193887d71c7088

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1499430 - Make PresShell::PageMove() use result of nsFrameSelection::GetFrameToPageSelect() when PresShell::GetScrollableFrameToScroll() returns nullptr

If there is no scrollable frame, PresShell::GetScrollableFrameToScroll() returns
nullptr.  However, even when we don't expand selection, we need to move caret
in current selection root.  Therefore, it should call
nsFrameSelection::CommonPageMove() with the result of
nsFrameSelection::GetFrameToPageSelect() to move caret.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49720e0b63617c2348087dd6fa739c5d4cc2e61d

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87e21b003b07
Make PresShell::PageMove() use result of nsFrameSelection::GetFrameToPageSelect() when PresShell::GetScrollableFrameToScroll() returns nullptr r=smaug

Comment 14

[Jorg K (CEST = GMT+2)]

Can I have an m-esr60 version for this, please.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Jorg K (GMT+1) from comment #14)

Can I have an m-esr60 version for this, please.

Cannot it be applied directly? It's really small patch?

Comment 16

[Jorg K (CEST = GMT+2)]

Not quite, there seems to be an issue in the test, well, I tried applying to our THUNDERBIRD_60_VERBRANCH where I have already backported bug 1482425.

$ hg qpush
applying 87e21b003b07
patching file layout/base/tests/mochitest.ini
Hunk #1 FAILED at 145
1 out of 1 hunks FAILED -- saving rejects to file layout/base/tests/mochitest.ini.rej
patching file layout/base/tests/test_moving_and_expanding_selection_per_page.html
Hunk #1 FAILED at 17
1 out of 2 hunks FAILED -- saving rejects to file layout/base/tests/test_moving_and_expanding_selection_per_page.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 87e21b003b07

But the reject file is small.

BTW, this isn't strictly a small patch, is it? 664 lines, 37 KB.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Wow, really odd. The test change is big, but the test was new for bug 1482425. So, only the different points should be mochitest.ini and PresShel.cpp, but they are enough small.

Have not you applied the patch for bug 1499996? It fixed the random orange of the new test for bug 1482425, though. (You usually see random oranges of the test?)

Comment 18

[Jorg K (CEST = GMT+2)]

I've never heard of bug 1499996. Maybe it would be best if you did an ESR60 patch for me, so I don't mess it up ;-)

You can do: hg up -C THUNDERBIRD_60_VERBRANCH

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, okay, the patch for bug 1499996 has already been merged. The difference is how to get docShell.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'll attach a patch after build and running the test, but I'm not sure if I can attach it before going to bed.

Comment 21

[Jorg K (CEST = GMT+2)]

Thanks, absolutely no rush :-)

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34be3d74074ec69778882bfc020b6bed1862ea8d

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch for ESR60

Here is!

Comment 24

[Jorg K (CEST = GMT+2)]

Thanks, I'll get it landed for TB 60.5 ESR!

Comment 25

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/87e21b003b07

Comment 26

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/2218e4b1e2cdd28586e1bcc0ca191034a119d5e9 on THUNDERBIRD_60_VERBRANCH