Can't check in to flight on aircanada.ca

RESOLVED FIXED in Firefox 59

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: overholt, Assigned: emilio)

Tracking

({regression, site-compat})

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 unaffected, firefox59 fixed)

Details

Attachments

(2 attachments)

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.
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).
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.
Hmm, but those dates don't line up with those revisions (https://hg.mozilla.org/mozilla-central/pushloghtml?rev=37e0bd91&rev=ec14d683).
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.
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
Blocks: 1035091
Component: DOM → CSS Parsing and Computation
Flags: needinfo?(emilio)
Thanks, Alice!
You need to be logged in to repro, right? Going to the "Check in now" page I can fill my first name just fine.
Flags: needinfo?(overholt)
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)
Flags: needinfo?(overholt)
Posted file 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?
Flags: needinfo?(emilio) → needinfo?(bugs)
Attachment #8936689 - Attachment mime type: text/plain → text/html
(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 :)
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?
Flags: needinfo?(bugs)
(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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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 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.
Attachment #8936714 - Flags: review?(bugs) → review-
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).
Attachment #8936714 - Flags: review- → review?(bugs)
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.
Attachment #8936714 - Flags: review?(bugs)
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?
Attachment #8936714 - Flags: review?(bugs) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2ad057a99aae
Decide which frame to focus using the flattened tree. r=smaug
Assignee: nobody → emilio
Flags: needinfo?(bugs) → needinfo?(emilio)
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b83870070900
Decide which frame to focus using the flattened tree. r=smaug
Made the testcase a bit more reliable so it didn't intermittently fail.
Flags: needinfo?(emilio)
Sigh, the test is so flaky... :((.

I'll try to make it a bit more robust...
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d2322e0d24ef
Decide which frame to focus using the flattened tree. r=smaug
https://hg.mozilla.org/mozilla-central/rev/d2322e0d24ef
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Flags: needinfo?(emilio)
I just verified I can check-in in AirCanada again :)
Thank you!
You need to log in before you can comment on or make changes to this bug.