Remove textbox value saving

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: enndeakin, Assigned: enndeakin)

Tracking

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Posted patch Remove textbox value caching (obsolete) — Splinter Review

I assume the original intent of this was to save the value when the element is removed or changed, then reinserted. This relies on the box object, which could be deleted in some cases when the element is removed anyway, and only saves in the specific cases where the destructor is called.

Since it only works in some cases, for now at least, just remove this, as it is the only code that relies on the boxobject get/setProperty methods.

Depends on: 1519960
No longer depends on: 1519960
Priority: -- → P3
Attachment #9036409 - Flags: review?(paolo.mozmail)
Comment on attachment 9036409 [details] [diff] [review]
Remove textbox value caching

Looks good to me, please wait until after the soft code freeze for landing.

I don't expect many regressions, but they'll be easy to fix if any is discovered.
Attachment #9036409 - Flags: review?(paolo.mozmail) → review+

So a change made in bug 1517480 now relies on the behaviour being removed by this patch, so I have added some simple support for it.

Attachment #9046026 - Flags: review?(dao+bmo)
Attachment #9036409 - Attachment is obsolete: true
Attachment #9046028 - Flags: review?(MattN+bmo)
Comment on attachment 9046028 [details] [diff] [review]
Part 2 - set value property in login manager as well as attribute

Review of attachment 9046028 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/LoginManagerPrompter.jsm
@@ +888,5 @@
>        chromeDoc.getElementById("password-notification-username")
>                 .setAttribute("placeholder", usernamePlaceholder);
> +      let nameField = chromeDoc.getElementById("password-notification-username");
> +      nameField.setAttribute("value", login.username);
> +      nameField.value = login.username;

Can you maybe update the comment above this? I guess once <textbox> is converted to CE this problem will go away.
Attachment #9046028 - Flags: review?(MattN+bmo) → review+
Attachment #9046026 - Flags: review?(dao+bmo) → review+
Pushed by neil@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63908a1890a0
remove textbox value resetting that relying on box objects. This in some cases prevents the value from restoring but it doesn't restore in all useful cases anyway, r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/57624be2e55c
fix login manager to not rely on the password field to not yet be initialized. This was broken by the other patch for bug 1519957, but using the .value property is what it should be doing anyway, but leave the attribute setting just in case, r=mattn
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.