Closed
Bug 491221
Opened 15 years ago
Closed 15 years ago
default button styling missing in the bookmarks properties dialog
Categories
(Firefox :: Bookmarks & History, defect)
Tracking
()
VERIFIED
FIXED
Firefox 3.6a1
People
(Reporter: dao, Assigned: dao)
References
(Depends on 1 open bug)
Details
(Keywords: access, regression, Whiteboard: [polish-easy] [polish-interactive][polish-p3])
Attachments
(2 files, 1 obsolete file)
6.51 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
1.90 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
With the fix for bug 462662, the "Save" button isn't highlighted as the default button anymore, although Enter will still save changes.
Assignee | ||
Comment 1•15 years ago
|
||
dialog.xml doesn't do the accept action if event.getPreventDefault() is true, which is the case for the cases that the current code tries to handle. So all that stuff isn't necessary anymore. Similarly to how the above works, browser-places.js should use event.getPreventDefault().
Assignee | ||
Updated•15 years ago
|
Whiteboard: [polish-easy] [polish-interactive]
Updated•15 years ago
|
Attachment #375544 -
Flags: review?(dietrich) → review-
Comment 2•15 years ago
|
||
Comment on attachment 375544 [details] [diff] [review] patch >diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js >--- a/browser/base/content/browser-places.js >+++ b/browser/base/content/browser-places.js >@@ -60,17 +60,17 @@ var StarUI = { > // Edit-bookmark panel > get panel() { > delete this.panel; > var element = this._element("editBookmarkPanel"); > // initially the panel is hidden > // to avoid impacting startup / new window performance > element.hidden = false; > element.addEventListener("popuphidden", this, false); >- element.addEventListener("keypress", this, true); >+ element.addEventListener("keypress", this, false); > return this.panel = element; > }, > > // list of command elements (by id) to disable when the panel is opened > _blockedCommands: ["cmd_close", "cmd_closeWindow"], > _blockCommands: function SU__blockCommands() { > for each(var key in this._blockedCommands) { > var elt = this._element(key); >@@ -107,35 +107,26 @@ var StarUI = { > this._uri = null; > if (this._batching) { > PlacesUIUtils.ptm.endBatch(); > this._batching = false; > } > } > break; > case "keypress": >- if (aEvent.keyCode == KeyEvent.DOM_VK_ESCAPE) { >- // If the panel is visible the ESC key is mapped to the cancel button >- // unless we are editing a folder in the folderTree, or an >- // autocomplete popup is open. >- if (!this._element("editBookmarkPanelContent").hidden) { >- var elt = aEvent.target; >- if ((elt.localName != "tree" || !elt.hasAttribute("editing")) && >- !elt.popupOpen) >+ if (aEvent.getPreventDefault()) >+ break; a small comment on what happens here would be appreciated I tested this a bit, everything looks fine in the star dialog, while in the dialog there's a problem with the following STR: 1. open 2 tabs and choose Bookmarks All Tabs (alternatively in the history sidebar or on a link choose bookmark this link) 2. open the folder selector 3. double click on a folder name to edit it 4. while editing press ESC to cancel the change or ENTER to confirm the name change 5. the dialog wrongly closes We need to add a test to browser_bookmarksProperties.js to check for this case, since i missed it somehow.
Comment 3•15 years ago
|
||
sorry i meant: "everything works fine in the Star Panel, while in bookmarks properties dialog there's a problem"...
Assignee | ||
Comment 4•15 years ago
|
||
Marco found out that this doesn't yet work for editable trees as it should. This fixes it.
Attachment #375588 -
Flags: review?(enndeakin)
Assignee | ||
Updated•15 years ago
|
Attachment #375544 -
Flags: review- → review?(mak77)
Updated•15 years ago
|
Attachment #375544 -
Flags: review?(mak77) → review+
Comment 5•15 years ago
|
||
Comment on attachment 375544 [details] [diff] [review] patch >diff --git a/browser/base/content/browser-places.js b/browser/base/content/browser-places.js >@@ -107,35 +107,26 @@ var StarUI = { > this._uri = null; > if (this._batching) { > PlacesUIUtils.ptm.endBatch(); > this._batching = false; > } > } > break; > case "keypress": >- if (aEvent.keyCode == KeyEvent.DOM_VK_ESCAPE) { >- // If the panel is visible the ESC key is mapped to the cancel button >- // unless we are editing a folder in the folderTree, or an >- // autocomplete popup is open. >- if (!this._element("editBookmarkPanelContent").hidden) { >- var elt = aEvent.target; >- if ((elt.localName != "tree" || !elt.hasAttribute("editing")) && >- !elt.popupOpen) >+ if (aEvent.getPreventDefault()) >+ break; as said before, please add a small comment on what is handling this condition. r=me with the additional fix to tree.xml widget landed at the same time. i filed bug 491269 to add a test for checking that editing our folder tree on bookmark properties dialog does not close the dialog, i'll take care of such a test, trying to land it before this change.
Comment 6•15 years ago
|
||
Comment on attachment 375588 [details] [diff] [review] tree.xml fix FYI the moving of the stopPropagation calls doesn't look right to me.
Assignee | ||
Comment 7•15 years ago
|
||
(In reply to comment #6) > FYI the moving of the stopPropagation calls doesn't look right to me. Why?
Comment 8•15 years ago
|
||
Hmm, I had a vague memory of Enter on the thread pane doing something in Thunderbird, but I can't find the code that does it, so I can't tell whether the tree.xml change would regress that or not. Sorry.
Assignee | ||
Comment 9•15 years ago
|
||
It opens/closes a container and also opens it in a new window. That's buggy, and this patch would fix it. But now that I look at it again, the event should only be consumed if changeOpenState returns true.
Assignee | ||
Comment 10•15 years ago
|
||
That's done in ThreadPaneKeyPress, btw: http://mxr.mozilla.org/comm-central/source/suite/mailnews/threadPane.js#218
Assignee | ||
Comment 11•15 years ago
|
||
see previous comment
Attachment #375588 -
Attachment is obsolete: true
Attachment #375615 -
Flags: review?(enndeakin)
Attachment #375588 -
Flags: review?(enndeakin)
Comment 12•15 years ago
|
||
(In reply to comment #10) > That's done in ThreadPaneKeyPress, btw: > http://mxr.mozilla.org/comm-central/source/suite/mailnews/threadPane.js#218 D'oh, I searched for ENTER instead of RETURN, silly me.
Updated•15 years ago
|
Attachment #375615 -
Flags: review?(enndeakin) → review+
Assignee | ||
Comment 13•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/5c05c9d7c710 I had to undo some of the browser-places.js clean-up, as it wasn't working for the expanders and for the New Folder button (bug 493218) as it should -- no idea why.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
Comment 14•15 years ago
|
||
Dao, i see another problem on trunk only, so please if you can confirm file a new regression on that, or tell me. If i start typing a tag and press Enter to accept the suggestion (notice i don't move to the autocomplete popup) the dialog closes. So: 1. right click on a bookmark and choose properties 2. add a "test" tag (type test and blur the tags field) 2. type "te" in the tags field 3. press Enter to confirm the autocomplete suggestion Dialog closes (should not) i can reproduce only on trunk, since this did not made branch i think it's probably a regression
Assignee | ||
Comment 15•15 years ago
|
||
Well, yeah. That's really how the widget works. Autofill doesn't ask for a conformation. You can tab away or use another keyboard shortcut and the value persists.
Comment 16•15 years ago
|
||
so if i confirm an autocomplete suggestion i automatically confirm the dialog? We need to fix that, sadly. it should confirm the suggestion but don't close the edit bookmark dialog... the previous fix included this.
Assignee | ||
Comment 17•15 years ago
|
||
(In reply to comment #16) > so if i confirm an autocomplete suggestion i automatically confirm the dialog? I'm saying that the auto-filled value can't be confirmed. It can be deleted or overwritten and otherwise it stays. All autofill textboxes work like this -- Enter operates regardless of whether the value has been typed or auto-filled. I'm not going to dictate you how to deal with this in the bookmarks panel, but what you're seeing currently on trunk is the standard behavior.
Comment 18•15 years ago
|
||
Dao, do you have an example in the UI where that happen? I can only see it for the location bar and search bar. The difference to the tag autocomplete in the bookmarks panel is that the latter one accepts multiple by comma separated values. Closing the dialog by hitting Enter will stop this way of tagging.
Assignee | ||
Comment 19•15 years ago
|
||
Only in Thunderbird. Firefox doesn't use autofill anywhere else.
Comment 20•15 years ago
|
||
Ok, but so we have different premises. When I confirm the autocompletion in the address field the cursor jumps to the next address field. So that's intentional. But inside the bookmarks properties dialog we only have one field which can contain a couple of tags. So we should really fix this in bug 494530. Marking this bug as verified for the other fixes with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090526 Minefield/3.6a1pre ID:20090526031623
Status: RESOLVED → VERIFIED
Comment 21•15 years ago
|
||
This bug's priority relative to the set of other polish bugs is: P3 - Polish issue that is in a secondary interface, occasionally encountered, or is not easily identifiable.
Whiteboard: [polish-easy] [polish-interactive] → [polish-easy] [polish-interactive][polish-p3]
Comment 22•15 years ago
|
||
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•