Closed Bug 1519957 Opened 3 years ago Closed 3 years ago

Remove textbox value saving


(Core :: XUL, enhancement, P3)




Tracking Status
firefox67 --- fixed


(Reporter: enndeakin, Assigned: enndeakin)




(2 files, 1 obsolete file)

Attached 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
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
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
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.