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.
Created attachment 364584 [details] Patch
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?
(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.
(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”>
(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😄
Oh wow, I'll try that.
(In reply to Daniel Bates from comment #4) > <input type=“text” style=“display: none”> Good tip! This worked.
Created attachment 365618 [details] Patch
Wenson, Daniel, does this look OK?
Any reviewer, please?
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.
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.
(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 on attachment 365618 [details] Patch Clearing flags on attachment: 365618 Committed r243647: <https://trac.webkit.org/changeset/243647>
All reviewed patches have been landed. Closing bug.
<rdar://problem/49422616>
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.
Using setValueForUser works on the handful of sites I just tested, and fixes ting.com, so great tip, thanks. Bug #196402