Closed Bug 1525935 Opened 6 years ago Closed 5 years ago

Only on first load, autofocus button is not focused

Categories

(Core :: DOM: Core & HTML, defect, P2)

67 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- verified

People

(Reporter: Fanolian+BMO, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

This is a follow-up of bug 1524080.

The I accept the risk button is not focused only on the first load after opening Nightly, no matter how many tabs/content processes or how long Nightly has been opened before about:config. Subsequent loads have the button focused just as what bug 1524080 was trying to fix (among the other accessibility fixes).

Steps to reproduce

  1. Make sure browser.aboutConfig.showWarning is true.
  2. Restart browser, either in main profile or a new profile.
  3. Load about:config. Observe the I accpet the risk button.
  4. Reload the page. Observe the button again.

Actual result
The first time about:config is loaded, I accept the risk is not focused.

Blocks: 1524080
Has Regression Range: --- → yes
Has STR: --- → yes

Just found out that the warning page of about:networking exhibits the same issue after bug 1525574 (it was never focused before it). Therefore this bug is not a regression from bug 1524080. I do not know which component this bug should be moved to though.

Maybe Layout :: Form Controls, which is the same component as bug 546995.

Status: UNCONFIRMED → NEW
Component: Preferences → Layout: Form Controls
Ever confirmed: true
Product: Toolkit → Core
Summary: Only on first load, "I accept the risk" button is not focused in new about:config → Only on first load, autofocus button is not focused

Sean, can you please have someone look into this issue which is a 67 regression (soft freeze on Mar 11)?

Flags: needinfo?(svoisen)
Blocks: 1493439

Hi, i am willing to work on it, please assign it to me.

Johann, I just wanted to leave a note that there is a platform bug here, and possibly now a front-end fix with the attached patch. If the patch fixes the symptom, then we might take it and you may want to follow up to make sure it is uplifted.

However, we shouldn't lose track of the underlying bug. I'm not sure if it affects web content as well. If that's the case, we may need to uplift a platform fix anyways. I see that there is an existing needinfo for Sean to look into this.

Flags: needinfo?(jhofmann)

Since this is also happening on about:networking, it looks like bug 1524080 isn't the regressor? What makes you so sure this is a recent regression on DOM side? Can someone find the regression range of this by testing about:networking with mozregression?

Flags: needinfo?(jhofmann)

So it seems like about:networking was also only changed very recently to have the autofocus attribute with bug 1525574. I have no idea what to do here and will defer to Sean to figure out if this is a DOM issue or what.

Also, I have no idea why that patch fixes it (it does, I tested it). Jawad, can you please let us know what made you come up with your patch?

Thanks!

Blocks: 1525574
Flags: needinfo?(ijawadak)

I don't believe this is a layout issue. Despite its categorization, per bug 546995, implementation of autofocus is in the DOM team's purview, so it sounds like someone on DOM might be better suited to look into this.

hsin-yi: Does this sound right to you?

Component: Layout: Form Controls → DOM: Core & HTML
Flags: needinfo?(svoisen) → needinfo?(htsai)

Since Hsin-Yi is away till 3/26, switching the ni to Henri to respond to comment 7.

Flags: needinfo?(htsai) → needinfo?(hsivonen)

(In reply to Neha Kochar [:neha] from comment #11)

Since Hsin-Yi is away till 3/26, switching the ni to Henri to respond to comment 7.

I'm not familiar with this code, but bug 712130 looks like a potential source of regression, so redirecting ni to patch author and reviewer from there in case they have ideas.

Flags: needinfo?(mozilla)
Flags: needinfo?(hsivonen)
Flags: needinfo?(bzbarsky)

Here is the regression range:

Last good: 20190129211401
First bad: 20190130215539
Pushlog_url: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=efb5e5425e2bc9baebb2a86730adbccf68deaa87&tochange=754f9877d4baec49ba19f60b81fad85076aec5fc

Potential regressor: bug 1521765

While checking for the regression range, in the good Firefox build (2019.01.29) when first time about:config is loaded, I accept the risk button is in focused and in the first bad Firefox build (2019.01.30) the I accept the risk button is not focused.

@Jawad Ahmed mind taking a look and confirming this is the case?

Flags: needinfo?(ijawadak)
Flags: needinfo?(ijawadak)

Adding ni for Paolo as bug 1521765 is identified as the potential regressor in comment 13.

Flags: needinfo?(paolo.mozmail)

Henri is right; bug 712130 might be relevant here. The "first load vs not" thing suggests some sort of timing effect, as does the proposed patch attached to this bug.

Bug 1521765 is not really relevant here, and the regression range from comment 13 makes no sense because the behavior after that range is not the behavior observed in this bug. Bug 1521765 switched to a setup where there was no autofocus at first (bug 1524080 tracked that); this bug is about the fact that the fix for bug 1524080 fails to work in a specific instance.

I'll take a quick look at what's going on here later today.

OK, here's what's going on:

  1. We delay autofocus until after layout has started, as part of the fix for bug 712130.
  2. We ignore autofocus attempts after the load event has fired (though there is a comment
    in the code about how maybe we should stop doing that, since the spec doesn't have that check).

Normally the load event never fires before layout has started, for documents that we're planning to lay out at all. But in this case, we're ending up starting layout after the load event, because l10n is blocking layout (but not blocking the load event).

This seems a bit odd to me. Should we add/remove a load event blocker when mPendingInitialTranslation transitions from false to true and back? Or do we really want the load event to fire before l10n is done with the document?

Flags: needinfo?(zbraniecki)
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(mozilla)
Flags: needinfo?(ijawadak)
Flags: needinfo?(bzbarsky)

This seems a bit odd to me. Should we add/remove a load event blocker when mPendingInitialTranslation transitions from false to true and back? Or do we really want the load event to fire before l10n is done with the document?

I'm not strongly opinionated here. I would expect that load is blocked on layout, but if the spec says nothing about it, then I guess it shouldn't be, and the fact that layout is blocked on l10n should be irrelevant?

And since autofocus wants to be blocked on layout, should it be triggered by load which is not?

I would expect that load is blocked on layout

You can't do that, because there may never be any layout at all (e.g. you could be a display:none iframe).

and the fact that layout is blocked on l10n should be irrelevant

The real question is whether l10n should block the load event or not. Generally things that kick off network loads and whatnot before the load event fires block that event firing...

should it be triggered by load

We could do autofocus from the load event, sure, but if there's been no layout there is nothing to focus, so I suspect in this case (l10n blocking layout but not load) things would still not end up focused...

Can we add document.l10n.ready to the list of blockers for the autofocus?

Yes, but how will that help? The whole point is that autofocus is already happening "too late" after onload, so doesn't take effect. We need to either allow autofocus after load has fired or delay load for localization.

And again, what is the rationale for not having localization delay the load event? It really seems to me like conceptually it should.

And again, what is the rationale for not having localization delay the load event? It really seems to me like conceptually it should.

I'm ok with that!

Flags: needinfo?(zbraniecki)
Priority: -- → P2
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a63de11853c4 Localization should delay the load event firing when it's delaying layout start. r=zbraniecki

Looks like there's a preexisting bug in Fluent (filed as bug 1539714). Before my changes, this bug "just" meant that some documents never got localized. Now that localization blocks the load event, failure to localize means that the load event never fires.

Depends on: 1539714
Flags: needinfo?(bzbarsky)
Assignee: ijawadak → bzbarsky
Attachment #9051301 - Attachment is obsolete: true
Attachment #9051301 - Attachment is obsolete: false
Attachment #9051301 - Attachment is obsolete: true

I am marking this bug as wontfix for 67 as we are past the beta midpoint to release.

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:bzbarsky, could you have a look please?

Flags: needinfo?(bzbarsky)

This is blocked on an open bug and can't land until that bug is fixed. Please fix the bot, please.

Flags: needinfo?(bzbarsky)
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4406373426ff Localization should delay the load event firing when it's delaying layout start. r=zbraniecki
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Is this something we should consider for backport to 68 (along with bug 1539714) or can it ride the trains?

Flags: needinfo?(bzbarsky)

It should ride the trains, imo.

Flags: needinfo?(bzbarsky)
QA Whiteboard: [good first verify]

Following the STR from the descrition, i reproduced this issue using Fx 67.0a1 on Windows 10. I can confirm that this issue is fixed, I verified using Fx 69.0b13 on Windows, Ubuntu(18.04 LTS), and MacOS

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: