If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

default button styling missing in the bookmarks properties dialog

VERIFIED FIXED in Firefox 3.6a1

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {access, regression})

3.5 Branch
Firefox 3.6a1
access, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [polish-easy] [polish-interactive][polish-p3])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

9 years ago
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

9 years ago
Created attachment 375544 [details] [diff] [review]
patch

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)
(Assignee)

Updated

9 years ago
Whiteboard: [polish-easy] [polish-interactive]

Updated

9 years ago
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"...
(Assignee)

Comment 4

9 years ago
Created attachment 375588 [details] [diff] [review]
tree.xml fix

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

9 years ago
Attachment #375544 - Flags: review- → review?(mak77)

Updated

9 years ago
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.
(Assignee)

Updated

9 years ago
Depends on: 491269

Comment 6

9 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

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

Why?

Comment 8

9 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

9 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

9 years ago
That's done in ThreadPaneKeyPress, btw: http://mxr.mozilla.org/comm-central/source/suite/mailnews/threadPane.js#218
(Assignee)

Comment 11

9 years ago
Created attachment 375615 [details] [diff] [review]
tree.xml fix v2

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.

Updated

9 years ago
Attachment #375615 - Flags: review?(enndeakin) → review+
(Assignee)

Updated

9 years ago
Blocks: 493218
(Assignee)

Comment 13

9 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
Last Resolved: 9 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
(Assignee)

Comment 15

8 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.
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

8 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.

Updated

8 years ago
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.
(Assignee)

Comment 19

8 years ago
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.