Closed Bug 1161882 Opened 5 years ago Closed 5 years ago

"Add a Keyword for this search..." stops working


(Firefox :: General, defect)

40 Branch
Not set



Firefox 40
40.3 - 11 May
Tracking Status
firefox39 --- unaffected
firefox40 --- verified


(Reporter: alice0775, Assigned: mak)



(Keywords: regression)


(1 file, 1 obsolete file)

[Tracking Requested - why for this release]:

Steps To Reproduce:
1. Right-click in the search box in web page
   Ex. page :
2. Choose "Add a Keyword for this Search…"
3. Fill fields in dialog
4. Attempt to save

Actual Results:
Save button is disabled

Expected Results:
Save button should be enabled and click-able.
Assignee: nobody → mak77
Flags: qe-verify-
Flags: firefox-backlog+
Points: --- → 2
Attached patch patch v1 (obsolete) — Splinter Review
The problem is due to the fact we don't initialize anymore fields that are not shown, but in the add keyword case we need to initialize the location field even if we hide it.

For a little while I was tempted to just unhide the field and leave the code more coherent, but the generated urls are usually unreadable and that would increase the risk of user modyfing them wrongly, breaking the search. So I decided to restore the old behavior, I think the primary reason we decided to hide urls was exactly cause for this specific case they are not human readable.
Attachment #8602138 - Flags: review?(ttaubert)
Comment on attachment 8602138 [details] [diff] [review]
patch v1

ugh, for whatever reason this ends up showing the location field on query containers...
Attachment #8602138 - Flags: review?(ttaubert)
ah yeah, we should invoke showOrCollapse in any case...
Attached patch patch v1.1Splinter Review
Attachment #8602138 - Attachment is obsolete: true
Attachment #8602182 - Flags: review?(ttaubert)
Iteration: --- → 40.3 - 11 May
Comment on attachment 8602182 [details] [diff] [review]
patch v1.1

Review of attachment 8602182 [details] [diff] [review]:

How hard would it be to write a test for this? Could do this in a follow-up.
Attachment #8602182 - Flags: review?(ttaubert) → review+
it's not extremely hard, but it will depend on bug 1160326.
I think we should just add tests there for every regression caused by bug  	951651
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Depends on: 1163341
I don't think this needs to be tracked at this point since it's already fixed in 40 and the regression was caused in 40 (by bug 951651).
You need to log in before you can comment on or make changes to this bug.