Closed Bug 491221 Opened 11 years ago Closed 11 years ago

default button styling missing in the bookmarks properties dialog

Categories

(Firefox :: Bookmarks & History, defect)

3.5 Branch
defect
Not set

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)

With the fix for bug 462662, the "Save" button isn't highlighted as the default button anymore, although Enter will still save changes.
Attached patch patchSplinter Review
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: nobody → dao
Status: NEW → ASSIGNED
Attachment #375544 - Flags: review?(dietrich)
Whiteboard: [polish-easy] [polish-interactive]
Attachment #375544 - Flags: review?(dietrich) → review-
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.
sorry i meant: "everything works fine in the Star Panel, while in bookmarks properties dialog there's a problem"...
Attached patch tree.xml fix (obsolete) — Splinter Review
Marco found out that this doesn't yet work for editable trees as it should. This fixes it.
Attachment #375588 - Flags: review?(enndeakin)
Attachment #375544 - Flags: review- → review?(mak77)
Attachment #375544 - Flags: review?(mak77) → review+
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.
Depends on: 491269
Comment on attachment 375588 [details] [diff] [review]
tree.xml fix

FYI the moving of the stopPropagation calls doesn't look right to me.
(In reply to comment #6)
> FYI the moving of the stopPropagation calls doesn't look right to me.

Why?
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.
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.
Attached patch tree.xml fix v2Splinter Review
see previous comment
Attachment #375588 - Attachment is obsolete: true
Attachment #375615 - Flags: review?(enndeakin)
Attachment #375588 - Flags: review?(enndeakin)
(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.
Attachment #375615 - Flags: review?(enndeakin) → review+
Blocks: 493218
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: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
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
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.
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.
(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.
Depends on: 494530
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.
Only in Thunderbird. Firefox doesn't use autofill anywhere else.
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
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]
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.