283719 - "Add keyword for this Search..." can fail if a form uses a fieldset (example: bugzilla advanced query, "e.type is not defined")

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[:Gavin Sharp [email: [e-mail]]]

Try to define a keyword to the bugzilla advanced query page doesn't work, since
the fieldset element has no "type" property.

Patch coming up.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch

Comment 2

[:Gavin Sharp [email: [e-mail]]]

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

Comment 3

[Conrad Carlen (not reading bugmail)]

> *** Bug 292384 has been marked as a duplicate of this bug. ***
Sorry, should have done a better bugsearch first.

The patch should work, but why set it to an empty string:
+      var type = (e.type) ? e.type.toLowerCase() : '';
and then continue with all the string comparisons below? It would be at least as
efficient, and more readable, to rule out things which can't be successful
controls (by checking for name was well) and then use 'continue' at the top of
the loop.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 175597 [details] [diff] [review]
Patch

You're right, this could be improved further. <select multiple>s should work as
well. New patch coming up.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch 2

This fixes <select multiple>s, and deals with fieldsets. I tested bugzilla
query pages for all affected elements, and google, works fine using both POST
and GET.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch 3

Improves upon the previous patch. Again, this allows <select multiple>s to
work, in addition to removing the exception thrown when dealing with forms with
fieldsets (like the bugzilla advanced query page). It's been tested with the
form at http://www.johnkeiser.com/cgi-bin/mozform.pl to ensure that it provides
the same output as Firefox 1.0.x, using both GET and POST, in addition to
testing that <select>s and <select multiple>s work correctly. With this patch,
it's also possible to make a keyword out of an advanced query on Bugzilla.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 188815 [details] [diff] [review]
Patch 3

Oops, Mano pointed out that I've inadvertently re-added a localName check.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch v4

Comment 9

[Asa Dotzler [:asa]]

we'd take a fully reviewed patch if the risk is low but we're not gonna block on
this. 

Comment 10

[Adam Guthrie]

Is this not a dupe of bug 263379? (Or vice versa.)

Comment 11

[:Gavin Sharp [email: [e-mail]]]

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

Comment 12

[Phil Ringnalda (:philor)]

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

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Given that this breaks the "Add a keyword for this field" feature on
http://www.wikipedia.org, and that the fix is relatively straightforward, asking
for blocking1.8b5 reconsideration.

Comment 14

[Asa Dotzler [:asa]]

might consider a super-safe patch but we're not gonna block on this.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Trunk: mozilla/browser/base/content/browser.js; new revision: 1.517;

Comment 16

[Asa Dotzler [:asa]]

So what's the risk of this patch and what areas would need to be tested before
we could be confident that this doesn't break anything?

Comment 17

[:Gavin Sharp [email: [e-mail]]]

This patch has been tested pretty extensively to ensure that it preserves the
old behavior (see comment 6) while fixing the specific issue with fieldsets. It
only affects the "Add a keyword for this search" option on text input context
menus, so testing out that command on various sites (Bugzilla, Yahoo, Wikipedia,
Google) and ensuring the resulting bookmark keyword works as it should is the
only thing there is to test.

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 188825 [details] [diff] [review]
Patch v4

OK, we're in exposure limiting mode so this isn't gonna make it for 1.5b2

Comment 19

[Adam Platti]

Should this bug be marked as resolved if it falied to make it into FF 1.5?

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #19)
> Should this bug be marked as resolved if it falied to make it into FF 1.5?

Yes. The Bugzilla "Status" field tracks the state of the bug on the trunk. Flags are used to indicate state on branches.

Comment 21

[Mike Connor [:mconnor]]

Comment on attachment 188825 [details] [diff] [review]
Patch v4

well baked, might as well get this on branch sooner rather than later.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Landed on the 1.8 branch for Firefox 2. (This code will likely be moved/modified when the new search service is landed).

mozilla/browser/base/content/browser.js; new revision: 1.479.2.45;

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060822 BonEcho/2.0b2. This is broken on the trunk due to places (bug 329281), but was fixed there previously.

Comment 24

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change