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

VERIFIED FIXED in Firefox 2 alpha1

Status

()

Firefox
Bookmarks & History
P3
normal
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

({verified1.8.1})

Trunk
Firefox 2 alpha1
verified1.8.1
Points:
---
Bug Flags:
blocking1.8b5 -
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

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.
Created attachment 175597 [details] [diff] [review]
Patch
Attachment #175597 - Flags: review?(mconnor)
Status: NEW → ASSIGNED
Priority: -- → P3
Summary: "Add keyword for this Search..." can fail if a form uses a fieldset (bugzilla advanced query, "e.type is not defined") → "Add keyword for this Search..." can fail if a form uses a fieldset (example: bugzilla advanced query, "e.type is not defined")
Whiteboard: [patch-r?]
Target Milestone: --- → Firefox1.1
Component: General → Bookmarks
QA Contact: general → mconnor
Flags: blocking-aviary1.1?
*** Bug 292384 has been marked as a duplicate of this bug. ***
> *** 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 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.
Attachment #175597 - Attachment is obsolete: true
Attachment #175597 - Flags: review?(mconnor)
Created attachment 182548 [details] [diff] [review]
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.
Attachment #182548 - Flags: review?(mconnor)
Attachment #182548 - Attachment is obsolete: true
Attachment #182548 - Flags: review?(mconnor)
Created attachment 188815 [details] [diff] [review]
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.
Attachment #188815 - Flags: review?(mconnor)
Comment on attachment 188815 [details] [diff] [review]
Patch 3

Oops, Mano pointed out that I've inadvertently re-added a localName check.
Attachment #188815 - Attachment is obsolete: true
Attachment #188815 - Flags: review?(mconnor)
Created attachment 188825 [details] [diff] [review]
Patch v4
Attachment #188825 - Flags: review?(mconnor)

Updated

13 years ago
Flags: blocking-aviary1.1? → blocking1.8b4+

Comment 9

12 years ago
we'd take a fully reviewed patch if the risk is low but we're not gonna block on
this. 
Flags: blocking1.8b4+ → blocking1.8b4-

Comment 10

12 years ago
Is this not a dupe of bug 263379? (Or vice versa.)
*** Bug 263379 has been marked as a duplicate of this bug. ***
*** Bug 309284 has been marked as a duplicate of this bug. ***
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.
Flags: blocking1.8b5- → blocking1.8b5?

Comment 14

12 years ago
might consider a super-safe patch but we're not gonna block on this.
Flags: blocking1.8b5? → blocking1.8b5-

Updated

12 years ago
Attachment #188825 - Flags: review?(mconnor) → review+
Whiteboard: [patch-r?] → [checkin needed]
Trunk: mozilla/browser/base/content/browser.js; new revision: 1.517;
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Attachment #188825 - Flags: approval1.8b5?

Comment 16

12 years ago
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?
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.

Updated

12 years ago
Keywords: qawanted

Comment 18

12 years ago
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
Attachment #188825 - Flags: approval1.8b5? → approval1.8b5-
Flags: blocking-aviary2?
Attachment #188825 - Flags: approval1.8.1?

Comment 19

12 years ago
Should this bug be marked as resolved if it falied to make it into FF 1.5?
(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 on attachment 188825 [details] [diff] [review]
Patch v4

well baked, might as well get this on branch sooner rather than later.
Attachment #188825 - Flags: approval1.8.1? → approval1.8.1+

Updated

12 years ago
Flags: blocking-firefox2? → blocking-firefox2+
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;
Keywords: qawanted → fixed1.8.1
Target Milestone: Firefox1.5 → Firefox 2
Target Milestone: Firefox 2 → Firefox 2 alpha1
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.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → bookmarks
You need to log in before you can comment on or make changes to this bug.