| Summary: | [iOS] Unable to make a selection in jsfiddle.net using arrow keys when requesting desktop site | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||
| Component: | Forms | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||
| Status: | REOPENED --- | ||||||||||||||||
| Severity: | Normal | CC: | bdakin, cdumez, commit-queue, corsar89, dbates, rniwa, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Wenson Hsieh
2019-01-23 20:59:24 PST
This also repros on coderpad.io. (In reply to Wenson Hsieh from comment #0) > To reproduce: (Step 0: use an iPad with a hardware keyboard!) > 1. Install iOS 12 > 2. Go to jsfiddle.net and request the desktop site > 2. Type text in any of the HTML/JS/CSS editors > 3. Using cmd+A or shift+arrow keys, attempt to make a "selection" > > Observe that the "selection" is immediately dismissed. Created attachment 359995 [details]
Patch for EWS
This is ridiculous. Comment on attachment 359995 [details] Patch for EWS View in context: https://bugs.webkit.org/attachment.cgi?id=359995&action=review > Source/WebCore/html/HTMLTextFormControlElement.cpp:-195 > - // We don't want to select all the text on iOS. Instead use the standard textfield behavior of going to the end of the line. That said, isn't this comment true? We don't generally expect selection to appear out of thin air when you select an input. I assume that /doesn't/ happen? Comment on attachment 359995 [details] Patch for EWS View in context: https://bugs.webkit.org/attachment.cgi?id=359995&action=review >> Source/WebCore/html/HTMLTextFormControlElement.cpp:-195 >> - // We don't want to select all the text on iOS. Instead use the standard textfield behavior of going to the end of the line. > > That said, isn't this comment true? We don't generally expect selection to appear out of thin air when you select an input. I assume that /doesn't/ happen? Well, it's true that we don't want to select all the text on iOS when focusing a text field. The way I understand it, Element.focus() is analogous to the platform's -becomeFirstResponder, and both of these don't select the entire contents of the text field. I /think/ this is what this comment was attempting to convey... However, select() is different from focus() in that it does not have a direct UIKit analogy; instead, its behavior is determined by the HTML spec, and should definitely adhere to that definition. I suppose I'm not entirely sure what it means to "select an input" in the context of the comment! (In reply to Wenson Hsieh from comment #7) > Comment on attachment 359995 [details] > Patch for EWS > > View in context: > https://bugs.webkit.org/attachment.cgi?id=359995&action=review > > >> Source/WebCore/html/HTMLTextFormControlElement.cpp:-195 > >> - // We don't want to select all the text on iOS. Instead use the standard textfield behavior of going to the end of the line. > > > > That said, isn't this comment true? We don't generally expect selection to appear out of thin air when you select an input. I assume that /doesn't/ happen? > > Well, it's true that we don't want to select all the text on iOS when > focusing a text field. The way I understand it, Element.focus() is analogous > to the platform's -becomeFirstResponder, and both of these don't select the > entire contents of the text field. I /think/ this is what this comment was > attempting to convey... > > However, select() is different from focus() in that it does not have a > direct UIKit analogy; instead, its behavior is determined by the HTML spec, > and should definitely adhere to that definition. > > I suppose I'm not entirely sure what it means to "select an input" in the > context of the comment! So digging into this a bit further, it looks like: - This patch would make it so that calling focus() on iOS will select the text content of a text field, if the selection hasn't been set previously in the text field. This makes iOS behavior match behavior in macOS. - However, on macOS, native NSTextFields actually exhibit this same behavior (becoming first responder selects all the text in the field!) - On iOS, when native UITextFields become first responder, we don't select the entire text in the UITextField. ...so on iOS, we probably ought to still have focus() match platform behavior (i.e. -[UITextField becomeFirstResponder]). Of course, the select() method should still select all the contents of the text field, since that's what the HTML spec mandates. Created attachment 360037 [details]
v2
Comment on attachment 360037 [details] v2 Clearing flags on attachment: 360037 Committed r240452: <https://trac.webkit.org/changeset/240452> All reviewed patches have been landed. Closing bug. *** Bug 193040 has been marked as a duplicate of this bug. *** Comment on attachment 360037 [details] v2 View in context: https://bugs.webkit.org/attachment.cgi?id=360037&action=review > Source/WebCore/html/HTMLInputElement.cpp:472 > +#if PLATFORM(IOS_FAMILY) We should tie this to the iOS editing behavior instead of a compile time flag like this. (In reply to Ryosuke Niwa from comment #13) > Comment on attachment 360037 [details] > v2 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=360037&action=review > > > Source/WebCore/html/HTMLInputElement.cpp:472 > > +#if PLATFORM(IOS_FAMILY) > > We should tie this to the iOS editing behavior instead of a compile time > flag like this. Good catch. Reopening to make this adjustment... Created attachment 361025 [details]
Use EditingBehavior
Comment on attachment 361025 [details] Use EditingBehavior View in context: https://bugs.webkit.org/attachment.cgi?id=361025&action=review > Source/WebCore/html/HTMLInputElement.cpp:474 > + auto frame = makeRefPtr(document().frame()); Is it necessary to take out a RefPtr for this? This seems like some next-level craziness or paranoia if anyone along this chain to shouldMove...() were to cause frame destruction and the world might as well end if shouldMove...() is the one that causes it. Not a big thing in the grand scheme of things, but definitely not minimal. Comment on attachment 361025 [details] Use EditingBehavior View in context: https://bugs.webkit.org/attachment.cgi?id=361025&action=review > Source/WebCore/html/HTMLInputElement.cpp:479 > setSelectionRange(start, std::numeric_limits<int>::max(), direction, revealMode, Element::defaultFocusTextStateChangeIntent()); Why don't we just bail out early if we don't have a frame? Does setSelectionRange() do this? If it does, why not bail out before calling it? Comment on attachment 361025 [details] Use EditingBehavior View in context: https://bugs.webkit.org/attachment.cgi?id=361025&action=review >> Source/WebCore/html/HTMLInputElement.cpp:474 >> + auto frame = makeRefPtr(document().frame()); > > Is it necessary to take out a RefPtr for this? This seems like some next-level craziness or paranoia if anyone along this chain to shouldMove...() were to cause frame destruction and the world might as well end if shouldMove...() is the one that causes it. Not a big thing in the grand scheme of things, but definitely not minimal. Fair enough. It's not incorrect to use a raw pointer here. But note that in the future, if any code were to run after calling setSelectionRange() that uses this raw Frame pointer, it would be a potential UAF. >> Source/WebCore/html/HTMLInputElement.cpp:479 >> setSelectionRange(start, std::numeric_limits<int>::max(), direction, revealMode, Element::defaultFocusTextStateChangeIntent()); > > Why don't we just bail out early if we don't have a frame? Does setSelectionRange() do this? If it does, why not bail out before calling it? setSelectionRange() does not bail early if frame is null. But as far as I can tell, the only state change it makes (besides updating layout) is calling `cacheSelection`. So it /seems/ like it should be safe to bail early, both in setSelectionRange and all of the helper methods that wrap it (e.g. setSelectionStart, setSelectionEnd, setSelectionDirection, select, setDefaultSelectionAfterFocus...). Regardless, this seems like a bigger change than is necessary for this patch, which is just about removing the compile-time guard. How do you feel about making this a separate change? Comment on attachment 361025 [details] Use EditingBehavior View in context: https://bugs.webkit.org/attachment.cgi?id=361025&action=review >>> Source/WebCore/html/HTMLInputElement.cpp:474 >>> + auto frame = makeRefPtr(document().frame()); >> >> Is it necessary to take out a RefPtr for this? This seems like some next-level craziness or paranoia if anyone along this chain to shouldMove...() were to cause frame destruction and the world might as well end if shouldMove...() is the one that causes it. Not a big thing in the grand scheme of things, but definitely not minimal. > > Fair enough. It's not incorrect to use a raw pointer here. But note that in the future, if any code were to run after calling setSelectionRange() that uses this raw Frame pointer, it would be a potential UAF. FYI, not really opposed to the makeRefPtr(). I think it's silly in this case, but r=me I am not going to stop you for it. I don't know anymore what is good engineering practice for dealing with this. UAF is bad. However, this paranoia you have for code after calling setSelection..()... that's setSelection...()'s problem. That function should take out the ref if it needs lifetime guarantees not its caller or it's caller's caller or its caller's caller's caller or 50 caller's up. If we're at that level of craziness then garbage collecting the world doesn't seem half bad. I don't want to be responsible for a UAF and I don't have a checkout at the moment to audit the code to say with any level confidence other than gut feeling that we won't have a UAF. But I can say this... the code here doesn't take out a ref of the frame for setSelection...() so we have the situation you describe then we have a UAF now. I say, fix setSelec..() and everyone else that needs lifetime guarantees. >>> Source/WebCore/html/HTMLInputElement.cpp:479 >>> setSelectionRange(start, std::numeric_limits<int>::max(), direction, revealMode, Element::defaultFocusTextStateChangeIntent()); >> >> Why don't we just bail out early if we don't have a frame? Does setSelectionRange() do this? If it does, why not bail out before calling it? > > setSelectionRange() does not bail early if frame is null. But as far as I can tell, the only state change it makes (besides updating layout) is calling `cacheSelection`. So it /seems/ like it should be safe to bail early, both in setSelectionRange and all of the helper methods that wrap it (e.g. setSelectionStart, setSelectionEnd, setSelectionDirection, select, setDefaultSelectionAfterFocus...). Regardless, this seems like a bigger change than is necessary for this patch, which is just about removing the compile-time guard. > > How do you feel about making this a separate change? No preference. Just wanted ask. Created attachment 361054 [details]
Patch for landing
Comment on attachment 361054 [details] Patch for landing Clearing flags on attachment: 361054 Committed r240926: <https://trac.webkit.org/changeset/240926> Comment on attachment 361054 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=361054&action=review > Source/WebCore/html/HTMLInputElement.cpp:475 > + if (frame && frame->editor().behavior().shouldMoveSelectionToEndWhenFocusingTextInput()) { Document has settings. We should use that. I *think* this has a bug that an input element whose document is not in a frame won't respect this iOS behavior. I *think* selectionStart & selectionEnd are always observable on an input element regardless of whether the element is in a document or its document is detached from frame. Comment on attachment 361054 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=361054&action=review >> Source/WebCore/html/HTMLInputElement.cpp:475 >> + if (frame && frame->editor().behavior().shouldMoveSelectionToEndWhenFocusingTextInput()) { > > Document has settings. We should use that. > I *think* this has a bug that an input element whose document is not in a frame won't respect this iOS behavior. > I *think* selectionStart & selectionEnd are always observable on an input element > regardless of whether the element is in a document or its document is detached from frame. Okay. Definitely sounds worth addressing. I think there are three ways to do this, and I'm not sure which is preferable. Do you have any opinions about whether we should: (A) Add an editingBehavior() getter to Settings, or (B) Add an editingBehavior() getter to Document, or (C) Check directly against editingBehaviorType(), and not go through EditingBehavior? (In reply to Wenson Hsieh from comment #23) > Comment on attachment 361054 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=361054&action=review > > >> Source/WebCore/html/HTMLInputElement.cpp:475 > >> + if (frame && frame->editor().behavior().shouldMoveSelectionToEndWhenFocusingTextInput()) { > > > > Document has settings. We should use that. > > I *think* this has a bug that an input element whose document is not in a frame won't respect this iOS behavior. > > I *think* selectionStart & selectionEnd are always observable on an input element > > regardless of whether the element is in a document or its document is detached from frame. > > Okay. Definitely sounds worth addressing. I think there are three ways to do > this, and I'm not sure which is preferable. Do you have any opinions about > whether we should: > (A) Add an editingBehavior() getter to Settings, or > (B) Add an editingBehavior() getter to Document, or > (C) Check directly against editingBehaviorType(), and not go through > EditingBehavior? Oh, interesting. editingBehavior is on Editor but the code seems to indicate that we only need settings really: EditingBehavior Editor::behavior() const { return EditingBehavior(m_frame.settings().editingBehaviorType()); } We could even create a new EditingBehavior to ask the same question if we wanted. Alternatively, we can move it to Settings, or add a new getter on Settings. Comment on attachment 361054 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=361054&action=review >>>> Source/WebCore/html/HTMLInputElement.cpp:475 >>>> + if (frame && frame->editor().behavior().shouldMoveSelectionToEndWhenFocusingTextInput()) { >>> >>> Document has settings. We should use that. >>> I *think* this has a bug that an input element whose document is not in a frame won't respect this iOS behavior. >>> I *think* selectionStart & selectionEnd are always observable on an input element >>> regardless of whether the element is in a document or its document is detached from frame. >> >> Okay. Definitely sounds worth addressing. I think there are three ways to do this, and I'm not sure which is preferable. Do you have any opinions about whether we should: >> (A) Add an editingBehavior() getter to Settings, or >> (B) Add an editingBehavior() getter to Document, or >> (C) Check directly against editingBehaviorType(), and not go through EditingBehavior? > > Oh, interesting. editingBehavior is on Editor but the code seems to indicate that we only need settings really: > > EditingBehavior Editor::behavior() const > { > return EditingBehavior(m_frame.settings().editingBehaviorType()); > } > > We could even create a new EditingBehavior to ask the same question if we wanted. Alternatively, we can move it to Settings, or add a new getter on Settings. The last plan sounds good — I'll add a new getter on Settings (plan (A) above) then, and post a patch shortly. Thanks! Comment on attachment 361054 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=361054&action=review >>>>> Source/WebCore/html/HTMLInputElement.cpp:475 >>>>> + if (frame && frame->editor().behavior().shouldMoveSelectionToEndWhenFocusingTextInput()) { >>>> >>>> Document has settings. We should use that. >>>> I *think* this has a bug that an input element whose document is not in a frame won't respect this iOS behavior. >>>> I *think* selectionStart & selectionEnd are always observable on an input element >>>> regardless of whether the element is in a document or its document is detached from frame. >>> >>> Okay. Definitely sounds worth addressing. I think there are three ways to do this, and I'm not sure which is preferable. Do you have any opinions about whether we should: >>> (A) Add an editingBehavior() getter to Settings, or >>> (B) Add an editingBehavior() getter to Document, or >>> (C) Check directly against editingBehaviorType(), and not go through EditingBehavior? >> >> Oh, interesting. editingBehavior is on Editor but the code seems to indicate that we only need settings really: >> >> EditingBehavior Editor::behavior() const >> { >> return EditingBehavior(m_frame.settings().editingBehaviorType()); >> } >> >> We could even create a new EditingBehavior to ask the same question if we wanted. Alternatively, we can move it to Settings, or add a new getter on Settings. > > The last plan sounds good — I'll add a new getter on Settings (plan (A) above) then, and post a patch shortly. > > Thanks! Introducing an editingBehavior getter on Settings is actually not straightforward, unless we add support for a new type of readonly entry in Settings.yaml, or add a virtual editingBehavior() method on SettingsBase. I'm going to put this getter on Document instead. Reopening to attach new patch. Created attachment 361112 [details]
Patch
Created attachment 361117 [details]
Patch
Have we confirmed the premise about selection being queryable from a frameless document? If so, why not add a test? I am kinda surprised since I seem to recall selection tied to the frame. (In reply to Daniel Bates from comment #31) > I am kinda surprised since I seem to recall selection tied to the frame. Right, but I think in this case it's about the input element's selectionStart and selectionEnd, which are saved in cached values even if the DOM selection is moved. I tried briefly to devise a test for this scenario, but couldn't. It essentially involved loading an iframe with a text field, saving a reference to the text field, disconnecting the frame, and then manipulating the text field to try and get bogus values for selectionStart and selectionEnd. ...but to no avail :/ Hm... yeah, all sorts of things appear to be broken once the document loses its frame. I think what's broken here isn't so much the cached selection of the input element but the focusing, etc... |