Don't start controlling a read-only input even if it's marked as a login manager field

RESOLVED FIXED in mozilla25

Status

()

Toolkit
Form Manager
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

Trunk
mozilla25
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Created attachment 779037 [details] [diff] [review]
v.1 Move the read-only check earlier

This way we can still mark a username field as one that login manager cares about without having to duplicate the read-only check in Password Manager. This also means we can keep a field marked as a login form despite @readonly so we don't need to watch for attribute changes in password manager.
Attachment #779037 - Flags: review?(dolske)
Try push: https://tbpl.mozilla.org/?tree=Try&rev=c795d3ab1896
Comment on attachment 779037 [details] [diff] [review]
v.1 Move the read-only check earlier

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

::: toolkit/components/satchel/nsFormFillController.cpp
@@ +863,5 @@
>  
>    nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(input);
>    if (isPwmgrInput || (formControl &&
>                         formControl->IsSingleLineTextControl(true) &&
> +                       (hasList || autocomplete))) {

A bit of driveby cleanup, but I think you could move the |formControl| checks up to the top too, which would simplify the conditional here.

I don't see how we'd ever get isPwmgrInput == true without IsSingleLineTextControl() also being true.
Attachment #779037 - Flags: review?(dolske) → review+
Created attachment 780144 [details] [diff] [review]
Part 2 - Cleanup nsFormFillController::Focus to not treat password fields specially

(In reply to Justin Dolske [:Dolske] from comment #2)
> A bit of driveby cleanup, but I think you could move the |formControl|
> checks up to the top too, which would simplify the conditional here.
> 
> I don't see how we'd ever get isPwmgrInput == true without
> IsSingleLineTextControl() also being true.

Yeah, I thought the same but didn't want to mix it into the same patch in order to make reviewing easier. Here is a simple patch that cleans the rest up. Form Manager and Password Manager tests pass locally.
Attachment #780144 - Flags: review?(dolske)
Attachment #780144 - Flags: review?(dolske) → review+
https://hg.mozilla.org/integration/fx-team/rev/10d4c53564bc
https://hg.mozilla.org/integration/fx-team/rev/cb81405573fe

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/10d4c53564bc
https://hg.mozilla.org/mozilla-central/rev/cb81405573fe
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.