256079 - Clicking focused readonly textbox triggers autocomplete

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Jesse Ruderman]

1. Load 
  data:text/html,<form action="http://www.google.com/"><input name="q" readonly>
2. Click the textbox twice.

Result: Autocomplete widget appears.

You can even use autocomplete to put text into the field, which is invisible but
submitted anyway.

Comment 1

[:deanis74]

My build machine is still in a shambles.  What adding this somewhere in
nsFormFillController::MouseClick()?  I think it should compile.

  if (!mFocusedInput)
     return NS_OK;

  nsCOMPtr<nsIDOMHTMLInputElement> input = do_QueryInterface(target);
  PRBool disabled = PR_FALSE;
  input->GetDisabled(&disabled);
  if (disabled)
    return NS_OK;

Comment 2

[Martijn Wargers (dead)]

Attachment: patch

I don't know, but isn't it better to add it in nsFormFillController::Focus?
This seems to work for me. No autocomplete anymore for readonly textboxes.

Comment 3

[:deanis74]

Comment on attachment 156743 [details] [diff] [review]
patch

>+    if (type.EqualsLiteral("text") && (isReadOnly == PR_FALSE) &&

&& (!isReadOnly)

Comment 4

[:deanis74]

That looks like the right approach to me.  If I had reviewing powers I'd r=me on
this, with the small change I mentioned.

Can someone review and check in?

Comment 5

[Martijn Wargers (dead)]

Attachment: patch2

Updated patch, addressing comment 3.

Comment 6

[:deanis74]

Comment on attachment 157009 [details] [diff] [review]
patch2

>+    if (type.EqualsLiteral("text") && (!isReadOnly) &&

I screwed up.  You just need !isReadOnly, without the brackets.  No need for a
new patch, though, that can be fixed on check-in.

Comment 7

[:deanis74]

Comment on attachment 157009 [details] [diff] [review]
patch2

Mike, is good to go?

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Not a "blocker" - if this gets review re-nominate. 

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 157009 [details] [diff] [review]
patch2

r=me, without the excess brackets as dean mentioned.  And since I'm here, lets
get this on the approval train.

Comment 10

[:deanis74]

Re-nominating as per comment 8.

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 157009 [details] [diff] [review]
patch2

>--- nsFormFillController.cpp	20 Aug 2004 20:34:37 -0000	1.34
>+++ nsFormFillController.cpp	25 Aug 2004 23:43:42 -0000
>@@ -507,22 +507,25 @@ nsFormFillController::Focus(nsIDOMEvent*
> {
>   nsCOMPtr<nsIDOMEventTarget> target;
>   aEvent->GetTarget(getter_AddRefs(target));
>   
>   nsCOMPtr<nsIDOMHTMLInputElement> input = do_QueryInterface(target);
>   if (!input)
>     return NS_OK;
>     
>+    PRBool isReadOnly = PR_FALSE;
>+    input->HasAttribute(NS_LITERAL_STRING("readonly"), &isReadOnly);

This should be:

input->GetReadOnly(&isReadOnly);

or it won't pay attention to dynamic changes.

Comment 12

[:deanis74]

Attachment: hand-edited patch

I hand-edited Martijn's patch to make the two changes.	If it looks good, I'll
make sure it applies properly before checking in.

Comment 13

[:deanis74]

Comment on attachment 160678 [details] [diff] [review]
hand-edited patch

I'm going to carry forward Mike's r= on this.

Comment 14

[chris hofmann]

looks like patches are coming.  we should get this in quick.

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 160678 [details] [diff] [review]
hand-edited patch

a=[e-mail]

Comment 16

[:deanis74]

Fixed on trunk and branch.

Comment 17

[TimoT]

My VS.NET 2003 build just crashed and burned with

 /cygdrive/x/mozilla/toolkit/components/satchel/src/nsFormFillController.cpp
nsFormFillController.cpp
x:\mozilla\toolkit\components\satchel\src\nsFormFillController.cpp(525)
: error C2039: 'EqualsLiteral' : is not a member of 'nsAutoString'
        x:\obj\dist\include\string\nsTString.h(532) : see declaration of
 'nsAutoString'
make[6]: *** [nsFormFillController.obj] Error 2
make[6]: Leaving directory `/cygdrive/x/mozilla/obj/toolkit/components/satchel/s
rc'
make[5]: *** [libs] Error 2

Comment 18

[:deanis74]

Tinderbox builds have been building fine since this check-in, and this check-in
didn't add any new string comparisons.  Based on the line number, you're dying at:

  !autocomplete.LowerCaseEqualsLiteral("off"))

but that wasn't changed with this check-in.  I think it's something else that's
causing your problem.

Comment 19

[:deanis74]

Timo: see my previous comment

Comment 20

[Guenter Huerkamp]

(In reply to comment #17)
> My VS.NET 2003 build just crashed and burned with
> 
>  /cygdrive/x/mozilla/toolkit/components/satchel/src/nsFormFillController.cpp
> nsFormFillController.cpp
> x:\mozilla\toolkit\components\satchel\src\nsFormFillController.cpp(525)
> : error C2039: 'EqualsLiteral' : is not a member of 'nsAutoString'
>         x:\obj\dist\include\string\nsTString.h(532) : see declaration of
>  'nsAutoString'
> make[6]: *** [nsFormFillController.obj] Error 2
> make[6]: Leaving directory `/cygdrive/x/mozilla/obj/toolkit/components/satchel/s
> rc'
> make[5]: *** [libs] Error 2

same error here, what can we do?

Comment 21

[Steffen Wilberg]

Erm, just compare this:
http://lxr.mozilla.org/aviarybranch/search?string=EqualsLiteral
(No matching files)

http://lxr.mozilla.org/seamonkey/search?string=EqualsLiteral
(Too many hits, displaying first 1000)

And no, there was no successful Firefox build since this checkin. Sweetlou and
Imalo are stuck.

Comment 22

[Steffen Wilberg]

Backed out on the branch.

You changed type.Equals to type.EqualsLiteral on the branch. But EqualsLiteral
isn't on the branch, see my comment above.

My build completes successfully with this backed out.

Comment 23

[:deanis74]

Well, it finally happened.  I broke a build.  My apologies to everyone for this
oversight.

I'm at work without my build machine.  Can someone check in this patch without
changing Equals to EqualsLiteral?  If not I'll do it tonight.

Comment 24

[Mike Connor [:mconnor]]

yeesh, I should have caught that, or bryner.  EqualsLiteral was introduced in
the 1.8a cycles by biesi.  Steffen, if you don't mind relanding this, I need to
get some code reviews done in a limited time window.. :)

Comment 25

[Steffen Wilberg]

Attachment: aviary patch, without EqualsLiteral

I'll land this as soon as there's at least one green Firefox branch tinderbox.

Comment 26

[Worcester12345]

(In reply to comment #0)
> 1. Load 
>   data:text/html,<form action="http://www.google.com/"><input name="q" readonly>
> 2. Click the textbox twice.
> 
> Result: Autocomplete widget appears.
> 
> You can even use autocomplete to put text into the field, which is invisible but
> submitted anyway.


Just stumbled upon this bug. I thought this was the desired behavior. I like it,
because you can find old search info there. I only wish there was a way to clear
it or "tune" it.

Comment 27

[Steffen Wilberg]

Comment on attachment 160755 [details] [diff] [review]
aviary patch, without EqualsLiteral

Bah.

Comment 28

[Steffen Wilberg]

Attachment: aviary patch

This one compiles.

Comment 29

[:deanis74]

Comment on attachment 160760 [details] [diff] [review]
aviary patch

Based on comment 24, this can be checked in.

Comment 30

[Steffen Wilberg]

Checked into branch again 2004-10-01 14:43.

Comment 31

[:deanis74]

Thanks for fixing my screw-up Steffen!

Comment 32

[Phil Ringnalda (:philor)]

*** Bug 265079 has been marked as a duplicate of this bug. ***

Comment 33

[Phil Ringnalda (:philor)]

*** Bug 265050 has been marked as a duplicate of this bug. ***

Comment 34

[José Jeria]

*** Bug 266278 has been marked as a duplicate of this bug. ***

Comment 35

[Martijn Wargers (dead)]

*** Bug 263331 has been marked as a duplicate of this bug. ***