Spec discussions - https://github.com/whatwg/html/issues/6909 Affected WPT tests - imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/show-picker-cross-origin-iframe.html imported/w3c/web-platform-tests/html/semantics/forms/the-input-element/show-picker.html
Created attachment 453186 [details] Patch
Created attachment 453214 [details] Patch
Need to rebase more progressed tests, at least web-platform-tests/html/dom/idlharness.https.html. I’ll wait to review until after that.
Created attachment 453414 [details] Patch
Comment on attachment 453414 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453414&action=review > Source/WebCore/html/ColorInputType.h:69 > + void openPopupView() final; WebCore::Chrome calls this operation "run the open panel", now this new function calls it "open the pop-up view", and the new public DOM standard function calls it "show the picker". I don’t think it’s valuable to have three different sets of terminology for this in WebKit. Can we get closer to one set of terminology? I don’t mind changing the existing ones if they should be changed. I’d prefer that this function not use a third term, if we can avoid it, unless there’s an important distinction we are expressing. > Source/WebCore/html/HTMLInputElement.cpp:1298 > + return Exception { SecurityError, "HTMLInputElement::showPicker() called from cross-origin iframe." }; Is this the format that we use for function names in these kinds of exceptions? HTMLInputElement::showPicker() looks natural to a C++ programmer, but maybe not so much to a web programmer using JavaScript?
<rdar://problem/89803367>
Created attachment 454459 [details] Patch
Comment on attachment 454459 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454459&action=review review- because of the incorrect handling of null frame > Source/WebCore/html/HTMLInputElement.cpp:1278 > + if (!m_inputType->isFileUpload() && !m_inputType->isColorControl() && frame) { Normally we would add a function to InputType for something like this named something like allowsShowPickerAcrossFrames. > Source/WebCore/html/HTMLInputElement.cpp:1284 > + auto* window = frame->window(); The code above checks frame for null, but then this deferences frame without a null check. I suggest we consider an early exit to silently and do nothing when the frame is null. Or treat a null frame the same as a null window perhaps.
Created attachment 454691 [details] Patch
(In reply to Darin Adler from comment #8) > Comment on attachment 454459 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=454459&action=review > > review- because of the incorrect handling of null frame > > > Source/WebCore/html/HTMLInputElement.cpp:1278 > > + if (!m_inputType->isFileUpload() && !m_inputType->isColorControl() && frame) { > > Normally we would add a function to InputType for something like this named > something like allowsShowPickerAcrossFrames. > > > Source/WebCore/html/HTMLInputElement.cpp:1284 > > + auto* window = frame->window(); > > The code above checks frame for null, but then this deferences frame without > a null check. I suggest we consider an early exit to silently and do nothing > when the frame is null. Or treat a null frame the same as a null window > perhaps. Thanks very much for the suggestions. The review comments have been addressed. PTAL. Thank you.
Comment on attachment 454691 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=454691&action=review Looks really good, but still some sticking points, sadly. > Source/WebCore/html/HTMLInputElement.cpp:1279 > + m_inputType->allowsShowPickerAcrossFrames(); This isn’t checking the result and returning it. Needs to be more like this: if (auto result = m_inputType->allowsShowPickerAcrossFrames(); result.hasException()) return result; And, we need to add test cases for this. The fact that no tests failed proves we aren’t covering this with tests at all. > Source/WebCore/html/InputType.cpp:520 > +ExceptionOr<void> InputType::allowsShowPickerAcrossFrames() What I was referring to was a boolean-returning "allowsShowPickerAcrossFrames" function overridden by file upload and color controls. So there is no isFileUpload and isColorControl function call. Would you consider that? I did *not* mean that all of this logic would be moved into the InputType base class.
Created attachment 455360 [details] Patch
Comment on attachment 455360 [details] Patch Looks good although it needs to be rebased. Now I am confused, though, about how showPicker needs cross frame checks for everything except file and color, but is only implemented for file and color. So what are the cases where showPicker needs to forbid cross-frame access, and how are we testing those cases? Is it about preparation for some future expansion?
Created attachment 455368 [details] Patch
(In reply to Darin Adler from comment #13) > Comment on attachment 455360 [details] > Patch > > Looks good although it needs to be rebased. Now I am confused, though, about > how showPicker needs cross frame checks for everything except file and > color, but is only implemented for file and color. So what are the cases > where showPicker needs to forbid cross-frame access, and how are we testing > those cases? Is it about preparation for some future expansion? Maybe check the top message (from @domenic) at https://github.com/whatwg/html/pull/7319#discussion_r759633854?
Read that, and that top message indicates that in *Chrome* they have other pickers besides these two. So the code makes sense assuming it was written *for* Chromium. In WebKit at the moment, it's dead code, untested and nearly untestable. Could test it because it presumably manifests as a slightly different exception when you call this on an input that has no picker. I’m not sure it’s great to have this dead code in WebKit, but not super harmful either.
Comment on attachment 455368 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=455368&action=review > Source/WebCore/ChangeLog:6 > + Reviewed by Darin Adler. Stray space here
Committed r291742 (248774@main): <https://commits.webkit.org/248774@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 455368 [details].
One more clarification we need here: Is it correct that when called on an HTMLInputElement of a type that has no picker, that showPicker should check cross-frame and user gesture status, throw exceptions if either of those applies, but then if neither applies, silently do nothing? That is the behavior we currently have implemented. Is that what we intend?
(In reply to Darin Adler from comment #19) > One more clarification we need here: > > Is it correct that when called on an HTMLInputElement of a type that has no > picker, that showPicker should check cross-frame and user gesture status, > throw exceptions if either of those applies, but then if neither applies, > silently do nothing? > > That is the behavior we currently have implemented. Is that what we intend? As per the first two points at https://github.com/whatwg/html/issues/6909#issuecomment-956747809, I guess it's intentional?
No, the comment talks about actually showing pickers. What I am asking about is where we do the cross-frame and user gesture checks, but then do *nothing* and do not show a picker. Doing nothing is not the same as showing a picker.
(In reply to Darin Adler from comment #21) > No, the comment talks about actually showing pickers. What I am asking about > is where we do the cross-frame and user gesture checks, but then do > *nothing* and do not show a picker. Doing nothing is not the same as showing > a picker. I'm not sure for the moment. Let me check on it.
*** Bug 234009 has been marked as a duplicate of this bug. ***
https://github.com/whatwg/html/issues/6909#issuecomment-1077449731
(In reply to Darin Adler from comment #21) > No, the comment talks about actually showing pickers. What I am asking about > is where we do the cross-frame and user gesture checks, but then do > *nothing* and do not show a picker. Doing nothing is not the same as showing > a picker. According to https://github.com/whatwg/html/issues/6909#issuecomment-1077776931, it seems that doing nothing is the intention.
OK, thanks for following up with the others on this.
FYI we are planning to make this method throw for non-mutable controls: see https://github.com/whatwg/html/issues/7767 and https://github.com/whatwg/html/pull/7769 . I will file a separate WebKit bug once the spec PR is reviewed and merged; by that time we will likely have web platform tests ready as well.