1611661 - Number inputs in Google calendar don't work right with arrow keys

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Google calendar behavior regression

BUILD: 2020-01-24 Mac nightly.

STEPS TO REPRODUCE:

1. Load https://calendar.google.com
2. Log in as needed (please let me know if you need a test account?)
3. Click on an empty space in the calendar.
4. In the resulting dialog, click "More options"
5. In the notifications section, click "Add notification"
6. Click in the number input that adds (e.g. for me it adds a dropdown that has "Notification" and "E-mail", then a number input that says "10", then a dropdown that has minutes/hours/days/weeks; I'm talking about the thing that says "10").
7. Try to use the left/right arrow keys to move the caret so you can edit the value.

EXPECTED RESULTS: Arrow keys work

ACTUAL RESULTS: When I try to use the arrow keys, the caret disappears. I then can't use backspace to edit the value.

ADDITIONAL INFORMATION: I have a default "3 hour before email notification" set up for one of my calendars, so adding events to that comes with the number input pre-populated with "3". When I click in that one, even after the "3", the caret is placed before the "3", not after it as I expected. So at a guess, we might be getting confused in nsTextControlFrame about the frame structure we expect inside ourselves. Possibly inside nsTextControlFrame::OffsetToDOMPoint or so, since that assumes things about our the number of descendants we can have?

On the other hand, I can't reproduce this with a vanilla <input type="number">, so maybe there's something weird going on with the Google Calendar case...

Regression range on nightlies is https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e7163954456087c31980de0c22fba01096bfadd8&tochange=5ba39736e74b8a072a63ee215545f89d5c2ec8c8 so tentatively assuming this is a regression from bug 981248.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case

I love the test-case reducer so much, made it so easy to figure out what was going on.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

So the bug is basically here, as IsTextInputFrame() returns false for nsNumberControlFrame.

But there's something more a bit off, I think, as otherwise I'd expect <input type=text style="user-select: none"> to behave similarly oddly, and it doesn't.

Anyhow too late for today, will fix tomorrow.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

So the user-select: none weirdness also happens with textarea:

data:text/html,<textarea style="user-select: none">Foo bar baz something that wraps.

You can't move the caret to the second line, effectively... This is intentional to allow the author to override the behavior, but our behavior is not particularly sane, so we may want to change that to match other browsers (and our behavior with <input type="text">, which is a bit accidental I presume.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

It works in data:text/html,<div contenteditable></div><textarea style="user-select: none">FooBar something that wraps</textarea>, though mostly by chance, lol. I'll fix this stuff up in bug 1611699.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1611661 - Give nsNumberControlFrame a TextInput type. r=jfkthame

LayoutFrameType::NumberControl is unused, and nsNumberControlFrame inherits now
from nsTextControlFrame. There are three places that check for
LayoutFrameType::TextInput. Two direct ones:

• ShouldApplyOverflowCLipping
• nsFontInflationData

And one indirect via IsTextInputFrame, the one mentioned in comment 2. For all
those three, it makes sense for nsNumberControlFrame to be handled in the same
way as nsTextControlFrame.

Long term we may want to get rid of the concept of frame types and just use
queryFrame or some sugar of that sort, as it's error prone. But for now this
fixes the bug.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1611661 - Tweak NODE_IS_EDITABLE flag handling for NAC so that it works consistently for <input type=number>. r=bzbarsky

I thought this would fix <input type=number style="user-select: none">, but
turns out it doesn't.

<input type=number> doesn't have the editor root as a root of the anonymous
subtree, so the current hack wouldn't work, as the anon root wouldn't have the
editable flag.

I had to tweak one AccessibleCaret test, but that's because it uses <textarea>
with user-select: none, and our behavior there is not particularly sane. It just
happened to work because that test-case also had a bunch of contenteditable
elements, and we stop matching this rule:

https://searchfox.org/mozilla-central/rev/220a3bd6063fcbe5ca50e88dcabdc7dee0aca448/layout/style/contenteditable.css#22

Because the anonymous div now properly matches :-moz-read-write, which made the
test work. See comment 4 of this bug.

I'll fix this stuff up and add some tests for our behavior here in bug 1611699.

Otherwise I don't think this is observable atm. I tried to remove only the frame
constructor code, and only two kinds of tests failed:

• Drag-and-drop tests: Dragging a number into an <input type=number> used to
  work before my patch. This patch alone doesn't fix it though, I'll
  investigate more in bug 1611701.

• Spellchecker tests: These are not relevant for <input type=number>.

Depends on D61088

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d7f2651eed8
Give nsNumberControlFrame a TextInput type. r=mats

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8d7f2651eed8

Comment 9

[Pascal Chevrel:pascalc]

Tracking as this regression affects a top web property. Is manual QA needed to verify the fix on Nightly?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Just verified manually, I don't think QA is needed but if you want to feel free to :)