1424633 - Can't check in to flight on aircanada.ca

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Andrew Overholt [:overholt]]

Clean profile in Nightly on Windows 10

1. Go to https://www.aircanada.com/ca/en/aco/home.html
2. Try to click in the 'FIRST NAME' field but can't

I'll try to run mozregression to figure out when this regressed.

Comment 1

[Andrew Overholt [:overholt]]

I forgot you have to click "Canada - English" on the language chooser thing and then click on "Check-in" (white on black "tab" towards the right).

Comment 2

[Andrew Overholt [:overholt]]

mozregression landed on 1421654 but that seems unrelated.

2017-11-30 was good and 2017-12-01 was bad and I'm pretty sure 37e0bd91 was good and ec14d683 was bad. I'll try more later.

Comment 3

[Andrew Overholt [:overholt]]

Hmm, but those dates don't line up with those revisions (https://hg.mozilla.org/mozilla-central/pushloghtml?rev=37e0bd91&rev=ec14d683).

Comment 4

[Andrew Overholt [:overholt]]

Looks like if you press <Tab> you can get focus to enter the 'FIRST NAME' field with a a cursor but it doesn't gain a cursor if you click into it.

Comment 5

[Alice0775 White]

The site uses a lot of @-moz-document in their css.

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4ad2150e73c0b19f1f6264a4b9ad6927986d43ce&tochange=37e0bd919af057d44c5c1410458c0f00a3653c11

Regressed by:
37e0bd919af0	Emilio Cobos Álvarez — Bug 1035091: Disable @-moz-document on author sheets on nightly and early beta. r=xidorn

Comment 6

[Andrew Overholt [:overholt]]

Thanks, Alice!

Comment 7

[Emilio Cobos Álvarez (:emilio)]

You need to be logged in to repro, right? Going to the "Check in now" page I can fill my first name just fine.

Comment 8

[Alice0775 White]

my str:
0. clear browsing history such as cache and cookie etc.
1. Go to https://www.aircanada.com/ca/en/aco/home.html , (no need  login)
2. click "Canada - English" on the language chooser thing
3. click on "Check-in" (white on black "tab" towards the right).
4. click on the placeholder text  'FIRST NAME' in the input field, (not empty area)

Comment 9

[Kohei Yoshino [:kohei]]

Mentioned this in the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2015/moz-document-support-will-be-dropped/

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: testcase

So, this is the bug they're working around.

When clicking on the absolutely positioned span, we don't dispatch focus to the parent element.

It's not clear to me that's expected. Olli, do you know what's supposed to happen here?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to Alice0775 White from comment #8)
> my str:
> 0. clear browsing history such as cache and cookie etc.
> 1. Go to https://www.aircanada.com/ca/en/aco/home.html , (no need  login)
> 2. click "Canada - English" on the language chooser thing
> 3. click on "Check-in" (white on black "tab" towards the right).
> 4. click on the placeholder text  'FIRST NAME' in the input field, (not
> empty area)

And, thanks a lot Alice, indeed :)

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

We tab navigate using frames, so I think in this case it leads to
https://searchfox.org/mozilla-central/rev/b0098afaeaad24a6752aa418ca3e86eade5fab17/dom/events/EventStateManager.cpp#3192-3195
What is the parent of an absolute positioned frame?

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] (won't be in Austin) from comment #12)
> We tab navigate using frames, so I think in this case it leads to
> https://searchfox.org/mozilla-central/rev/
> b0098afaeaad24a6752aa418ca3e86eade5fab17/dom/events/EventStateManager.
> cpp#3192-3195
> What is the parent of an absolute positioned frame?

Yeah, the parent of the abspos is the nearest positioned frame, so it's wrong.

Olli, is there a spec that defines this? I can fix this easily.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Well, hit testing itself isn't spec'ed.
But I think in this case when focus is happening using mouse, we shouldn't follow frames, but normal DOM propagation.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1424633: Decide which frame to focus using the flattened tree.

This is effectively the same than walking up the frame tree, which I think makes
sense, and it's also relatively easier to do in the current setup.

If you just want me to walk the flat tree up explicitly, or the light tree, just
let me know.

I had a hard time trying to test it, suggestions welcome.

Review commit: https://reviewboard.mozilla.org/r/207452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207452/

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936714 [details]
Bug 1424633: Decide which frame to focus using the flattened tree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207452/diff/1-2/

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

I'm thinking how this should be fixed... Flattened tree might be ok, or move tabindex handling to normal DOM event handling, in other words to EventTarget::PostHandleEvent.
The latter would be faster since we're already iterating event targets in the path.

(/me reviews some other patches first and gets back to this later.)

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8936714 [details]
Bug 1424633: Decide which frame to focus using the flattened tree.

https://reviewboard.mozilla.org/r/207452/#review213394

Ok, I think we should just move this to Element::PostHandleEvent. This is after all about mouse event handling, not sequental focus handling.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936714 [details]
Bug 1424633: Decide which frame to focus using the flattened tree.

So I tried to move it to PostHandleEvent, and I had a really bad time, in particular how to deal with suppressBlur and all that. Also, this wants to run regardless of mMultipleActionsPrevented, but only once, which would require adding a new event flag and that kind of stuff.

Would you be fine merging this for now? This is effectively walking the frame tree and skipping display: contents nodes (which have no frames and thus are not focusable).

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8936714 [details]
Bug 1424633: Decide which frame to focus using the flattened tree.

https://reviewboard.mozilla.org/r/207452/#review213538

Well, I can't review this since I don't know whether HasAnyStateBits(NS_FRAME_OUT_OF_FLOW)/GetPlaceholderFrame() captures all the cases we're interested in here.
In other words, does that follow the same path as what DOM event dispatch would follow.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936714 [details]
Bug 1424633: Decide which frame to focus using the flattened tree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207452/diff/2-3/

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8936714 [details]
Bug 1424633: Decide which frame to focus using the flattened tree.

https://reviewboard.mozilla.org/r/207452/#review213598

::: dom/events/EventStateManager.cpp:84
(Diff revision 3)
>  #include "nsIDOMXULDocument.h"
>  #include "nsIDragService.h"
>  #include "nsIDragSession.h"
>  #include "mozilla/dom/DataTransfer.h"
>  #include "nsContentAreaDragDrop.h"
> +#include "nsPlaceholderFrame.h"

leftover from previous patch?

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8936714 [details]
Bug 1424633: Decide which frame to focus using the flattened tree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207452/diff/3-4/

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ad057a99aae
Decide which frame to focus using the flattened tree. r=smaug

Comment 25

[Eliza Balazs [:ebalazs_]]

Backed out changeset 2ad057a99aae (bug 1424633) for failing 2 in dom/events/test/test_focus_abspos.html r=backout on a CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2ad057a99aaebbe26e311f74a9de24b45f241b78&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable


https://hg.mozilla.org/integration/autoland/rev/78bd64f9ee0892690c7a62b1a6a14b2e2081454d

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b83870070900
Decide which frame to focus using the flattened tree. r=smaug

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Made the testcase a bit more reliable so it didn't intermittently fail.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Here's a try run passing TV: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf06842008c6c7b2b7a75e56f3eec50f0877d475&selectedJob=151835085

Comment 29

[Cosmin Sabou [:CosminS]]

Backed out for multiple failures on dom/events/test/test_focus_abspos.html

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b83870070900d6d4c487b830e9b1342de474142d&filter-resultStatus=testfailed&selectedJob=151856106

https://treeherder.mozilla.org/logviewer.html#?job_id=151856106&repo=autoland&lineNumber=2664

https://hg.mozilla.org/integration/autoland/rev/e35b0ca176e15a08bff817d3474f277b207f211b

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Sigh, the test is so flaky... :((.

I'll try to make it a bit more robust...

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2322e0d24ef
Decide which frame to focus using the flattened tree. r=smaug

Comment 32

[Natalia Csoregi [:nataliaCs]]

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

Comment 33

[Emilio Cobos Álvarez (:emilio)]

I just verified I can check-in in AirCanada again :)

Comment 34

[Andrew Overholt [:overholt]]

Thank you!