Closed Bug 1645046 Opened 4 years ago Closed 4 years ago

Enable HTML5 dialog in Nightly

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla80
Tracking Flags:
Tracking Status
firefox80 --- fixed

People

(Reporter: sefeng, Assigned: sefeng)

References

Details

(Keywords: dev-doc-complete, perf-alert)

Attachments

(1 file)

Bug 1645046 - Enable HTML5 dialog in Nightly r=smaug
47 bytes, text/x-phabricator-request
Details | Review

Our implementation of dialog isn't perfect yet, there are 2 pieces that are missing.

  1. The inert element isn't supported (bug 921504).
  2. We currently use a temporary solution for the layout of modal dialog, we need some feedback from CSSWG to implement a finalized solution (bug 1637310).

Despite these 2 pieces, I think dialog is in a state where we can enable it in Nightly.

Severity: -- → S3
Priority: -- → P3
Assignee: nobody → sefeng
Status: NEW → ASSIGNED

Could you explain how our layout differs from other browsers?
And Is there a plan to support inert? (I guess technically bug 921504 isn't needed, but inert-ness itself.)

The main difference is our modal dialog is fixed, so it remains in centred during scrolling, and it's position: absolute in Chrome.

Well, the current plan (or what I am thinking) is to wait for bug 921504, however, I am open to discuss if we want to implement the inert-ness first to avoid waiting for bug 921504.

So there is this comment https://bugzilla.mozilla.org/show_bug.cgi?id=921504#c16
And https://bugzilla.mozilla.org/show_bug.cgi?id=1200896#c5
So it does sounds like we might need inertness as such even before possible inert attribute.

Marco, would you mind confirm that it's okay to enable dialog in Nightly from an a11y perspective?

Double-checking the team is okay with https://bugzilla.mozilla.org/show_bug.cgi?id=1200896#c5.

Thanks!

Flags: needinfo?(mzehe)

Yes, it is OK to enable this now, it gets exposed to accessibility APIs and therefore assistive technologies.

Flags: needinfo?(mzehe)
Pushed by sefeng@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b33756c2e334 Enable HTML5 dialog in Nightly r=smaug
Backout by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ee812b6359be Backed out changeset b33756c2e334 for causing several HTML related failures. CLOSED TREE
Pushed by sefeng@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/397ad8a55255 Enable HTML5 dialog in Nightly r=smaug
Pushed by sefeng@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0f5653007c41 Enable HTML5 dialog in Nightly r=smaug

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&searchStr=wpt&revision=dfe65c0d1d920d8b6f608b8dce09ebdd0aaa98bf

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=308506383&repo=autoland

backout link: https://hg.mozilla.org/integration/autoland/rev/86302753c7774ad2d31925ee37fe798bb72b242b

[task 2020-07-03T15:15:59.253Z] 15:15:59     INFO - mozversion platform_version: 80.0a1
[task 2020-07-03T15:16:09.694Z] 15:16:09     INFO - Closing logging queue
[task 2020-07-03T15:16:09.696Z] 15:16:09     INFO - queue closed
[task 2020-07-03T15:16:09.696Z] 15:16:09 CRITICAL - Test harness output was not a valid structured log message: 
[task 2020-07-03T15:16:09.696Z] 15:16:09 CRITICAL - Traceback (most recent call last):
[task 2020-07-03T15:16:09.697Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/runtests.py", line 16, in <module>
[task 2020-07-03T15:16:09.697Z] 15:16:09 CRITICAL -     rv = wptrunner.main()
[task 2020-07-03T15:16:09.697Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/tests/tools/wptrunner/wptrunner/wptrunner.py", line 412, in main
[task 2020-07-03T15:16:09.697Z] 15:16:09 CRITICAL -     return start(**kwargs)
[task 2020-07-03T15:16:09.697Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/tests/tools/wptrunner/wptrunner/wptrunner.py", line 396, in start
[task 2020-07-03T15:16:09.698Z] 15:16:09 CRITICAL -     rv = not run_tests(**kwargs) or logged_critical.has_log
[task 2020-07-03T15:16:09.698Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/tests/tools/wptrunner/wptrunner/wptrunner.py", line 187, in run_tests
[task 2020-07-03T15:16:09.698Z] 15:16:09 CRITICAL -     **kwargs)
[task 2020-07-03T15:16:09.699Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/tests/tools/wptrunner/wptrunner/wptrunner.py", line 89, in get_loader
[task 2020-07-03T15:16:09.700Z] 15:16:09 CRITICAL -     chunker_kwargs=chunker_kwargs)
[task 2020-07-03T15:16:09.700Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/tests/tools/wptrunner/wptrunner/testloader.py", line 245, in __init__
[task 2020-07-03T15:16:09.700Z] 15:16:09 CRITICAL -     self._load_tests()
[task 2020-07-03T15:16:09.700Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/tests/tools/wptrunner/wptrunner/testloader.py", line 308, in _load_tests
[task 2020-07-03T15:16:09.701Z] 15:16:09 CRITICAL -     for test_path, test_type, test in self.iter_tests():
[task 2020-07-03T15:16:09.701Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/tests/tools/wptrunner/wptrunner/testloader.py", line 301, in iter_tests
[task 2020-07-03T15:16:09.702Z] 15:16:09 CRITICAL -     yield test_path, test_type, self.get_test(manifest_file, test, inherit_metadata, test_metadata)
[task 2020-07-03T15:16:09.702Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/tests/tools/wptrunner/wptrunner/testloader.py", line 261, in get_test
[task 2020-07-03T15:16:09.702Z] 15:16:09 CRITICAL -     return wpttest.from_manifest(manifest_file, manifest_test, inherit_metadata, test_metadata)
[task 2020-07-03T15:16:09.702Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/tests/tools/wptrunner/wptrunner/wpttest.py", line 655, in from_manifest
[task 2020-07-03T15:16:09.702Z] 15:16:09 CRITICAL -     return test_cls.from_manifest(manifest_file, manifest_test, inherit_metadata, test_metadata)
[task 2020-07-03T15:16:09.702Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/tests/tools/wptrunner/wptrunner/wpttest.py", line 425, in from_manifest
[task 2020-07-03T15:16:09.703Z] 15:16:09 CRITICAL -     quic=quic)
[task 2020-07-03T15:16:09.703Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/tests/tools/wptrunner/wptrunner/wpttest.py", line 400, in __init__
[task 2020-07-03T15:16:09.703Z] 15:16:09 CRITICAL -     path, protocol, quic)
[task 2020-07-03T15:16:09.703Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/tests/tools/wptrunner/wptrunner/wpttest.py", line 168, in __init__
[task 2020-07-03T15:16:09.703Z] 15:16:09 CRITICAL -     self.environment = {"protocol": protocol, "prefs": self.prefs, "quic": quic}
[task 2020-07-03T15:16:09.704Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/tests/tools/wptrunner/wptrunner/wpttest.py", line 325, in prefs
[task 2020-07-03T15:16:09.704Z] 15:16:09 CRITICAL -     meta_prefs = meta.prefs
[task 2020-07-03T15:16:09.704Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/tests/tools/wptrunner/wptrunner/manifestexpected.py", line 444, in prefs
[task 2020-07-03T15:16:09.704Z] 15:16:09 CRITICAL -     return prefs(self)
[task 2020-07-03T15:16:09.704Z] 15:16:09 CRITICAL -   File "/builds/worker/workspace/build/tests/web-platform/tests/tools/wptrunner/wptrunner/manifestexpected.py", line 93, in prefs
[task 2020-07-03T15:16:09.704Z] 15:16:09 CRITICAL -     rv = dict(value(item) for item in node_prefs)
[task 2020-07-03T15:16:09.705Z] 15:16:09 CRITICAL - ValueError: dictionary update sequence element #0 has length 1; 2 is required
[task 2020-07-03T15:16:09.883Z] 15:16:09    ERROR - Return code: 1
[task 2020-07-03T15:16:09.885Z] 15:16:09    ERROR - No checks run.
[task 2020-07-03T15:16:09.885Z] 15:16:09    ERROR - No suite end message was emitted by this harness.
[task 2020-07-03T15:16:09.885Z] 15:16:09 CRITICAL - # TBPL FAILURE #
[task 2020-07-03T15:16:09.885Z] 15:16:09  WARNING - setting return code to 2
Pushed by sefeng@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/665f8f1a79a0 Enable HTML5 dialog in Nightly r=smaug

https://hg.mozilla.org/mozilla-central/rev/665f8f1a79a0

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox80: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla80

== Change summary for alert #26391 (as of Wed, 01 Jul 2020 03:24:23 GMT) ==

Improvements:

19% espn fcp android-hw-g5-7-0-arm7-api-16 opt cold 7,641.66 -> 6,161.42

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26391

Keywords: perf-alert
Flags: needinfo?(sefeng)
Regressions: 1651882

MDN docs completed for this change; see https://github.com/mdn/sprints/issues/3503#issuecomment-669868519 for full details.

Keywords: dev-doc-needed → dev-doc-complete

Sean,

An interesting point here on the way dialog behaves.
This site https://www.computerbase.de/ is using dialog but they set

    /Firefox\/83/.test(navigator.userAgent) && j.css('transform', 'translate(0, 0)'),

without the transform: translate(0px);, the dialog is shifted toward the top of the viewport and not usable.

Which led to a webcompat issue.
https://github.com/webcompat/web-bugs/issues/59492

Maybe there is something to double check on interop issues with Chrome.

Flags: needinfo?(sefeng)

This is expected.

The way we center the dialog is via top: 50%; transform: translateY(-50%); The site overwrites top to 1.5em, so that top: 1.5em; transform: translateY(-50%) causes the display issue.

Chrome's behaviour is known to be different than ours, they don't use transform at all and they have a hack in their code which basically hardcodes the value of top.

The fix they did also looks correct. The issue is for some reason it's not applied, which is something that I don't know. There's another issue though. <dialog> is enabled on Nightly, so simply testing Firefox/83 isn't sufficient.

Flags: needinfo?(sefeng)

Developer of the computerbase.de consent dialog here. I filed a Firefox bug a few days ago for the issue I saw (https://bugzilla.mozilla.org/show_bug.cgi?id=1669734) and applied a temporary workaround to our live site. At that time, I was assuming that this was a regression in Firefox 83 that would get fixed before it hits beta or stable channels which is why I used user-agent detection:

if (/Firefox/83/.test(navigator.userAgent)) {
dialog.css('transform', 'translate(0, 0)');
}

I didn't know at that time that this workaround would unset the default transform used by Firefox. Instead, my idea was to apply the transform to get around some broken "rendering fast-path" (maybe also strenghened by the broken-looking box-shadow rendering).

With the knowledge that Firefox applies a CSS transform to modal dialogs by default I have now fixed this issue properly. Some further changes were necessary to fix the "broken" (partially cut-off) box-shadow caused by Firefox applying "overflow: auto" to modal dialogs by default. Now everything seems to be fine in Firefox 83. :)

(It would be awesome if the default CSS applied to elements by Firefox was displayed by the developer tools, then I maybe would have seen the default transform.)

Hi Steffen,

Sounds perfect!

There's a browser style checkbox under Computed tab in developer tools, I think the default CSS can be seen there.

In case I wasn't clear for the checking Firefox 83 isn't sufficent issue. It's not sufficent because the Nightly version will change. After the next release, it 'll be Firefox 84 to have the same issue, then Firefox 85 and so on (Untill we've enabled it in all channels). You probably want to do a feature detection here, something like if (typeof dialog.showModal === "function") ..... Just to make sure we are on the same page.

I don't use user-agent detection anymore but thank you nevertheless. :)

(I avoid user-agent detection wherever possible. It was meant as a short-term stop-gap because I originally didn't know the root-cause.)

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

Attachment

General

Creator:
Created:
Updated:
Size: