Closed
Bug 1161882
Opened 10 years ago
Closed 10 years ago
"Add a Keyword for this search..." stops working
Categories
(Firefox :: General, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox39 | --- | unaffected |
firefox40 | --- | verified |
People
(Reporter: alice0775, Assigned: mak)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
[Tracking Requested - why for this release]:
Steps To Reproduce:
1. Right-click in the search box in web page
Ex. page : https://bugzilla.mozilla.org/
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 | ||
Updated•10 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Flags: qe-verify-
Flags: firefox-backlog+
Assignee | ||
Updated•10 years ago
|
Points: --- → 2
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
ah yeah, we should invoke showOrCollapse in any case...
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8602138 -
Attachment is obsolete: true
Attachment #8602182 -
Flags: review?(ttaubert)
Updated•10 years ago
|
Iteration: --- → 40.3 - 11 May
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
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).
status-firefox39:
--- → unaffected
tracking-firefox40:
? → ---
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•