Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw on showPicker() when the <input> is not mutable #7769

Merged
merged 2 commits into from
Apr 21, 2022

Conversation

domenic
Copy link
Member

@domenic domenic commented Mar 31, 2022

Closes #7767.

(See WHATWG Working Mode: Changes for more details.)

@emilio, could you review this? Also maybe you are thinking of writing the tests? :)


/input.html ( diff )

@domenic domenic force-pushed the showPicker-disabled-readonly branch from 565dce6 to a6cdc8f Compare March 31, 2022 16:50
Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... I just realized that the spec already accounts for mutability, here: https://html.spec.whatwg.org/#show-the-picker,-if-applicable

So I guess the issue I filed is a Chromium bug. However I think I prefer throwing rather than silently doing nothing for this case, tbh.

Also, I just realized while going through the tests and spec that we do not enforce that the element is rendered, i.e., that you can call input.showPicker() on a detached element, and per spec that should show a picker (somewhere? some pickers do need positioning information).

I think we should consider also throwing on non-rendered elements, what do you think?

@domenic
Copy link
Member Author

domenic commented Mar 31, 2022

Oh, huh. I guess we considered that. Indeed I feel like louder failure is probably better. In this case I should delete the extra check from "show the picker, if applicable".

As for whether it's in the DOM or not, I think in #6909 (comment) we considered this and decided it was OK to show pickers for elements not in the DOM, since that generally works for existing elements. (Especially on mobile.)

@emilio
Copy link
Contributor

emilio commented Mar 31, 2022

Hmm, so color and file at least in Firefox are special because they use the OS window system and aren't positioned relative to the element. But other elements like date / datetime need an element to get positioned, and would feel a bit weird in the middle of nowhere, IMO, since there would be no OS window toolbar, etc.

@beaufortfrancois
Copy link

FYI I've started a Chromium CL at https://chromium-review.googlesource.com/c/chromium/src/+/3563070/

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 1, 2022
This CL makes sure showPicker() throws InvalidStateError DOMException
when the HTML input element is either disabled or readonly.

Spec: whatwg/html#7769
Bug: 939561
Change-Id: I3c3ad9be897beb17ac110092574331e3af80633c
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 1, 2022
This CL makes sure showPicker() throws InvalidStateError DOMException
when the HTML input element is either disabled or readonly.

Spec: whatwg/html#7769
Bug: 939561
Change-Id: I3c3ad9be897beb17ac110092574331e3af80633c
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 11, 2022
This CL makes sure showPicker() throws InvalidStateError DOMException
when the HTML input element is either disabled or readonly.

Spec: whatwg/html#7769
Bug: 939561
Change-Id: I3c3ad9be897beb17ac110092574331e3af80633c
aarongable pushed a commit to chromium/chromium that referenced this pull request Apr 12, 2022
This CL makes sure showPicker() throws InvalidStateError DOMException
when the HTML input element is either disabled or readonly.

Spec: whatwg/html#7769
Bug: 939561
Change-Id: I3c3ad9be897beb17ac110092574331e3af80633c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3563070
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Fr <beaufort.francois@gmail.com>
Cr-Commit-Position: refs/heads/main@{#991363}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 12, 2022
This CL makes sure showPicker() throws InvalidStateError DOMException
when the HTML input element is either disabled or readonly.

Spec: whatwg/html#7769
Bug: 939561
Change-Id: I3c3ad9be897beb17ac110092574331e3af80633c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3563070
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Fr <beaufort.francois@gmail.com>
Cr-Commit-Position: refs/heads/main@{#991363}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 12, 2022
This CL makes sure showPicker() throws InvalidStateError DOMException
when the HTML input element is either disabled or readonly.

Spec: whatwg/html#7769
Bug: 939561
Change-Id: I3c3ad9be897beb17ac110092574331e3af80633c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3563070
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Fr <beaufort.francois@gmail.com>
Cr-Commit-Position: refs/heads/main@{#991363}
@beaufortfrancois
Copy link

Chromium has been updated: https://chromiumdash.appspot.com/commit/ee9a0b48b84c1ad754ef263ac8d9288140694c4f
And I've updated web platform tests: web-platform-tests/wpt#33465

@domenic
Copy link
Member Author

domenic commented Apr 13, 2022

In this case I should delete the extra check from "show the picker, if applicable".

I went a different direction. Now "show the picker, if applicable" early-returns on some various error conditions. These are useful for callers like the input activation behavior. But showPicker() checks those same conditions, and throws explicit exceptions.

@domenic domenic requested a review from annevk April 13, 2022 21:39
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe mention that this includes an editorial change in the commit message as that was a bit confusing at first.

@domenic domenic merged commit 39775c1 into main Apr 21, 2022
@domenic domenic deleted the showPicker-disabled-readonly branch April 21, 2022 17:00
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 22, 2022
…s not mutable, a=testonly

Automatic update from web-platform-tests
Throw on showPicker() when the <input> is not mutable

This CL makes sure showPicker() throws InvalidStateError DOMException
when the HTML input element is either disabled or readonly.

Spec: whatwg/html#7769
Bug: 939561
Change-Id: I3c3ad9be897beb17ac110092574331e3af80633c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3563070
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Fr <beaufort.francois@gmail.com>
Cr-Commit-Position: refs/heads/main@{#991363}

--

wpt-commits: 1f6399f790ee4b6af88cdcd4eaab32319dc1f260
wpt-pr: 33465
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Apr 22, 2022
…s not mutable, a=testonly

Automatic update from web-platform-tests
Throw on showPicker() when the <input> is not mutable

This CL makes sure showPicker() throws InvalidStateError DOMException
when the HTML input element is either disabled or readonly.

Spec: whatwg/html#7769
Bug: 939561
Change-Id: I3c3ad9be897beb17ac110092574331e3af80633c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3563070
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Fr <beaufort.francois@gmail.com>
Cr-Commit-Position: refs/heads/main@{#991363}

--

wpt-commits: 1f6399f790ee4b6af88cdcd4eaab32319dc1f260
wpt-pr: 33465
DanielRyanSmith pushed a commit to DanielRyanSmith/wpt that referenced this pull request Apr 27, 2022
This CL makes sure showPicker() throws InvalidStateError DOMException
when the HTML input element is either disabled or readonly.

Spec: whatwg/html#7769
Bug: 939561
Change-Id: I3c3ad9be897beb17ac110092574331e3af80633c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3563070
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Fr <beaufort.francois@gmail.com>
Cr-Commit-Position: refs/heads/main@{#991363}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
This CL makes sure showPicker() throws InvalidStateError DOMException
when the HTML input element is either disabled or readonly.

Spec: whatwg/html#7769
Bug: 939561
Change-Id: I3c3ad9be897beb17ac110092574331e3af80633c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3563070
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Fr <beaufort.francois@gmail.com>
Cr-Commit-Position: refs/heads/main@{#991363}
NOKEYCHECK=True
GitOrigin-RevId: ee9a0b48b84c1ad754ef263ac8d9288140694c4f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Interaction between showPicker() and disabled / readonly.
4 participants