Bug 195708 – HTMLInputElement::setEditingValue should not fail if renderer doesn't exist

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 364584 [details]
Patch

[Wenson Hsieh]

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?

[Michael Catanzaro]

(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.

[Daniel Bates]

(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”>

[Daniel Bates]

(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😄

[Michael Catanzaro]

Oh wow, I'll try that.

[Michael Catanzaro]

(In reply to Daniel Bates from comment #4)
> <input type=“text” style=“display: none”>

Good tip! This worked.

[Michael Catanzaro]

Created attachment 365618 [details]
Patch

[Michael Catanzaro]

Wenson, Daniel, does this look OK?

[Michael Catanzaro]

Any reviewer, please?

[Wenson Hsieh]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

(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.

[WebKit Commit Bot]

Comment on attachment 365618 [details]
Patch

Clearing flags on attachment: 365618

Committed r243647: <https://trac.webkit.org/changeset/243647>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49422616>

[Michael Catanzaro]

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.

[Michael Catanzaro]

Using setValueForUser works on the handful of sites I just tested, and fixes ting.com, so great tip, thanks. Bug #196402