Bug 195708

Summary: HTMLInputElement::setEditingValue should not fail if renderer doesn't exist
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: FormsAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cgarcia, commit-queue, dbates, mcatanzaro, webkit-bug-importer, wenson_hsieh, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
See Also: https://bugs.webkit.org/show_bug.cgi?id=196402
Attachments:
Description Flags
Patch
none
Patch none

Description Michael Catanzaro 2019-03-13 15:28:13 PDT
HTMLInputElement::setEditingValue currently returns early if the element's renderer() is null. This is causing the Epiphany password manager to fail to remember passwords on https://www.geico.com/ except for navigations through page cache.

I traced this back to https://bugs.webkit.org/show_bug.cgi?id=85353#c15 trying to figure out why it was added in the first place and discovered it was to avoid some assertion, but I don't know which one, and there's definitely not any assertion hit nowadays in this case. Probably there are more guards checking if renderer() is null elsewhere in the code nowadays, closer to where it's really needed.
Comment 1 Michael Catanzaro 2019-03-13 15:31:23 PDT
Created attachment 364584 [details]
Patch
Comment 2 Wenson Hsieh 2019-03-13 16:00:57 PDT
Comment on attachment 364584 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364584&action=review

> Source/WebCore/ChangeLog:18
> +        No new test because I have no clue how this could possibly be tested. The change should be
> +        good as long as it doesn't break existing tests.

Could you use Internals.setEditingValue?
Comment 3 Michael Catanzaro 2019-03-13 16:53:52 PDT
(In reply to Wenson Hsieh from comment #2)
> Could you use Internals.setEditingValue?

That, or the GTK DOM API.

The bigger problem is I don't know how rendering works: we'd have to somehow figure out how to trigger this NULL renderer() case.
Comment 4 Daniel Bates 2019-03-13 21:42:11 PDT
(In reply to Michael Catanzaro from comment #3)
> (In reply to Wenson Hsieh from comment #2)
> > Could you use Internals.setEditingValue?
> 
> That, or the GTK DOM API.
> 
> The bigger problem is I don't know how rendering works: we'd have to somehow
> figure out how to trigger this NULL renderer() case.

<input type=“text” style=“display: none”>
Comment 5 Daniel Bates 2019-03-13 21:43:00 PDT
(In reply to Daniel Bates from comment #4)
> (In reply to Michael Catanzaro from comment #3)
> > (In reply to Wenson Hsieh from comment #2)
> > > Could you use Internals.setEditingValue?
> > 
> > That, or the GTK DOM API.
> > 
> > The bigger problem is I don't know how rendering works: we'd have to somehow
> > figure out how to trigger this NULL renderer() case.
> 
> <input type=“text” style=“display: none”>

Except without the fancy quotes😄
Comment 6 Michael Catanzaro 2019-03-14 08:12:17 PDT
Oh wow, I'll try that.
Comment 7 Michael Catanzaro 2019-03-21 14:28:28 PDT
(In reply to Daniel Bates from comment #4)
> <input type=“text” style=“display: none”>

Good tip! This worked.
Comment 8 Michael Catanzaro 2019-03-21 14:30:37 PDT
Created attachment 365618 [details]
Patch
Comment 9 Michael Catanzaro 2019-03-23 08:25:01 PDT
Wenson, Daniel, does this look OK?
Comment 10 Michael Catanzaro 2019-03-29 05:56:04 PDT
Any reviewer, please?
Comment 11 Wenson Hsieh 2019-03-29 08:09:33 PDT
As Zalan pointed out to me, it seems a bit strange to use setEditingValue. On Cocoa platforms, autofill uses setValueForUser which turns around and calls setValue.

AFAICT, setEditingValue is only used in GTK code and internal testing helpers, so the impact of this change is pretty limited. But now I'm also wondering if we could just use setValueForUser or setValue.
Comment 12 Michael Catanzaro 2019-03-29 08:42:55 PDT
I think it would probably work.

I think the reason we use setEditingValue is to dispatch the change event. When called from JS, JSHTMLInputElement::setJSHTMLInputElementValueAsNumberSetter calls HTMLInputElement::setValue with DispatchNoEvent, and there's no way around that, so we have to use a native API. But using setValueForUser would also dispatch the change event. webkit_dom_element_html_input_element_set_editing_value() is public API, but we could deprecate it and reimplement it using setValueForUser if that works better, and then get rid of setEditingValue entirely (since keeping it is pointless if nothing except the tests uses it).

I'll need to do more testing, so I'm going to land this as-is for now, but I'll have a follow-up soon.
Comment 13 Michael Catanzaro 2019-03-29 08:50:50 PDT
(In reply to Michael Catanzaro from comment #12)
> webkit_dom_element_html_input_element_set_editing_value() is public API

Surprise, it was added by me, in bug #163082. Shame I didn't leave more detail in the bug itself. I just wrote "these are needed to make autofill work properly with Angular forms." Wish I remembered what page was causing problems there.
Comment 14 WebKit Commit Bot 2019-03-29 09:11:08 PDT
Comment on attachment 365618 [details]
Patch

Clearing flags on attachment: 365618

Committed r243647: <https://trac.webkit.org/changeset/243647>
Comment 15 WebKit Commit Bot 2019-03-29 09:11:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2019-03-29 09:12:19 PDT
<rdar://problem/49422616>
Comment 17 Michael Catanzaro 2019-03-29 11:25:43 PDT
I found in https://gitlab.gnome.org/GNOME/epiphany/commit/e34c87c316ca16dfe608def225523d2fe5872fbb the justification for using setEditingValue:

"""
Honestly, I have no clue what I've done here, but it's what Chromium
does when it autofills a form, and it makes an Angular password form
work on one of our internal sites, so it's probably good right?
"""

So that's not great.

It jogged my memory as to which site to test this setValueForUser with, at least.
Comment 18 Michael Catanzaro 2019-03-29 11:54:43 PDT
Using setValueForUser works on the handful of sites I just tested, and fixes ting.com, so great tip, thanks. Bug #196402