Can't check in to flight on aircanada.ca

RESOLVED FIXED in Firefox 59

Status

()

defect
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: overholt, Assigned: emilio)

Tracking

({regression, site-compat})

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

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

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
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.
(Reporter)

Comment 1

a year ago
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).
(Reporter)

Comment 2

a year ago
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.
(Reporter)

Comment 3

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

Comment 4

a year ago
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

a year ago
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
(Assignee)

Updated

a year ago
Flags: needinfo?(emilio)
(Reporter)

Comment 6

a year ago
Thanks, Alice!
(Assignee)

Comment 7

a year ago
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)

Comment 8

a year ago
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)
(Reporter)

Updated

a year ago
Flags: needinfo?(overholt)
(Assignee)

Comment 10

a year ago
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)
(Assignee)

Updated

a year ago
Attachment #8936689 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 11

a year ago
(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)
(Assignee)

Comment 13

a year ago
(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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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

a year ago
mozreview-review
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-
(Assignee)

Comment 19

a year ago
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 20

a year ago
mozreview-review
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 hidden (mozreview-request)

Comment 22

a year ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 24

a year ago
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)

Updated

a year ago
Assignee: nobody → emilio
Flags: needinfo?(bugs) → needinfo?(emilio)

Comment 26

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b83870070900
Decide which frame to focus using the flattened tree. r=smaug
(Assignee)

Comment 27

a year ago
Made the testcase a bit more reliable so it didn't intermittently fail.
Flags: needinfo?(emilio)
(Assignee)

Comment 30

a year ago
Sigh, the test is so flaky... :((.

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

Comment 31

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d2322e0d24ef
Decide which frame to focus using the flattened tree. r=smaug

Comment 32

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d2322e0d24ef
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(Assignee)

Updated

a year ago
Flags: needinfo?(emilio)
(Assignee)

Comment 33

a year ago
I just verified I can check-in in AirCanada again :)
(Reporter)

Comment 34

a year ago
Thank you!
You need to log in before you can comment on or make changes to this bug.