830242 - [QAC generated] Double clicking a folder in the right side pane opens first item into the current tab. And sent the 'library' into the background.

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Mathew Bakken]

Steps to Reproduce
---------------------------------------
1. Open Firefox
2. Go under the Firefox menu
3. Click on history
4. Double-click on 'Today' 


What should have happened:
---------------------------------------


It should have showed your history from today.

What actually happened:
----------------------------------------
It showed the page I was currently viewing and sent the 'library' into the background.

Comment 1

[Mathew Bakken]

Using latest Nightly build
Windows 8 Pro

Comment 2

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/1761f4a9081c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130112 Firefox/21.0 ID:20130112002208
Bad:
http://hg.mozilla.org/mozilla-central/rev/d8599591d07c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130112 Firefox/21.0 ID:20130112122707
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1761f4a9081c&tochange=d8599591d07c

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c12c5e31d72f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130111 Firefox/21.0 ID:20130111122909
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/f37caebbb0dc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130111 Firefox/21.0 ID:20130111125007
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c12c5e31d72f&tochange=f37caebbb0dc

Regressed by:
f37caebbb0dc	Asaf Romano — Bug 827293 - The first list item in the downloads view should be selected when it's opened (was: Error: TypeError: view.selectNode is not a function). r=mak

Comment 3

[Alice0775 White]

This is not only history but also bookmarks.

Steps to reproduce:
1. Open Nightly with clean profile
2. Open Library Ctrl+Shift+B
3, Double click 'Bookmarks Toolbar' in the left side pane
4, Double click 'Most Visited' in the Right side pane

Actual results:
First bookmark item in the 'Most Visited' folder is opened in the current tab
And the main window got focus

Expected results:
'Most Visited' folder is expand. but not open first bookmark item in the folder.
Library should stay in front

Comment 4

[Alice0775 White]

Latest Aurora20.0a2 tnderbox build is also affected,

Comment 5

[Asaf Romano (gone)]

What happens here is that the double click first bubbles to the tree binding, that indeed opens the "Today" container. Then, however, it also caught by the listener in the organizer (minding you the event target doesn't change... it's still the treechildren element). At that point, the organizer figures the selectedNode was double clicked... not knowing the tree binding listener replaced the tree contents along with the selected node.

Comment 6

[Asaf Romano (gone)]

Attachment: trunk patch

In the keypress listener, the event is already consumed if the container was opened or closed:

      <handler event="keypress" keycode="VK_ENTER">
        if (this._handleEnter(event)) {
          event.stopPropagation();
          event.preventDefault();
        }
      </handler>
      <handler event="keypress" keycode="VK_RETURN">
        if (this._handleEnter(event)) {
          event.stopPropagation();
          event.preventDefault();
        }
      </handler>

Thus I think it's ok to do so also for the double click.

However, since this affects all trees, I'll keep this change trunk-only, and provide a workaround for Aurora.

Comment 7

[Asaf Romano (gone)]

Attachment: Aurora workaround

Comment 8

[Marco Bonardo [:mak]]

So the problem happens cause bug 827293 ended up setting the selectNode earlier? was the double click handler inactive before? What I'm missing from the whole thing is why this wasn't an issue before.

Comment 9

[Marco Bonardo [:mak]]

If I got this correctly, double click listeners sets a new place that loads a new content, now with bug 827293 we set the selection as soon as the new content is loaded, the event bubbles up and double click happens.  Before out code change the bubbling event was not finding selectedNode and thus skipping the handling. with the aurora workaround we set the selectedNode asynchronously, thus again the handler happens but it can't find a selectedNode.

Comment 10

[Marco Bonardo [:mak]]

Did you run tests with the change? we have some browser-chrome tests going through these iirc, and delaying the selection may be unexpected by them.

what if instead of selecting in the "load" method, we do in 
<handler event="focus">
iff there is no selection?
Should then basically restore the previous behavior.

Comment 11

[Asaf Romano (gone)]

Prior to that fix, the first time was only selected in the initial load, and that was arubaly a bug. Thus, when the double click listener in the organizaer was called there was no selected node, and we were not affected by the underlying issue.

What happens now is that the selected node changes as a result of the actions done by the tree binding double click handler. The organizer handler doesn't know that and acts as if the selected node was double clicked.

The trunk patch fixes this correctly, by consuming the event in the tree binding, just as it does for return keypress in the same situation. The aurora workaround fixes this by delaying the call to select.

Comment 12

[Asaf Romano (gone)]

(In reply to Marco Bonardo [:mak] from comment #10)
> Did you run tests with the change? we have some browser-chrome tests going
> through these iirc, and delaying the selection may be unexpected by them.

Not yet, will do. But not they shouldn't be expecting any selection afaict.
> 
> what if instead of selecting in the "load" method, we do in 
> <handler event="focus">
> iff there is no selection?

We should reselect the first item even if the focus did not change.

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 701712 [details] [diff] [review]
Aurora workaround

Review of attachment 701712 [details] [diff] [review]:
-----------------------------------------------------------------

r=me provided mochitest-chrome and browser-chrome tests in browser/components/places/tests and toolkit/components/places/tests are happy about this change and not expencting the tree to have immediate selection.
Even better to have a tryrun.

::: browser/components/places/content/tree.xml
@@ +118,5 @@
>            result.addObserver(treeView, false);
>            this.view = treeView;
>  
>            if (this.getAttribute("selectfirstnode") == "true" && treeView.rowCount > 0) {
> +            // Hack for bug  Bug 830242.

please also provide a brief description of the problem.

@@ +121,5 @@
>            if (this.getAttribute("selectfirstnode") == "true" && treeView.rowCount > 0) {
> +            // Hack for bug  Bug 830242.
> +            Services.tm.mainThread.dispatch(function() {
> +              treeView.selection.select(0);
> +            }, Components.interfaces.nsIThread.DISPATCH_NORMAL);

why not Ci, we use it everywhere in this file

Comment 14

[Neil Deakin]

Comment on attachment 701710 [details] [diff] [review]
trunk patch

This makes sense, although there are places such as Thunderbird's message list where double-clicking does something other than open/close the node. It might still work though if the handler is processed after the one that opens new messages.

Comment 15

[Asaf Romano (gone)]

browser/browser_library_panel_leak.js fails.

Comment 16

[Marco Bonardo [:mak]]

looks like delaying selection is also delaying initialization of the details pane

Comment 17

[Asaf Romano (gone)]

Attachment: alternative patch (for both trunk and Aurora)

Comment 18

[Asaf Romano (gone)]

Attachment: alternative patch (for both trunk and Aurora)

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 702358 [details] [diff] [review]
alternative patch (for both trunk and Aurora)

Review of attachment 702358 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the removed check for event.target in onClick

Comment 20

[Asaf Romano (gone)]

Attachment: for checkin

inbound is closed right now.

I'll file a toolkit bug for the underlying tree binding issue. I don't have cycles to test Thunderbird myself right now.

Comment 21

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dcd159e6cb5

Comment 22

[Asaf Romano (gone)]

I filed bug 830869 for the tree binding bug.

Comment 23

[Asaf Romano (gone)]

Comment on attachment 702394 [details] [diff] [review]
for checkin

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Download panel feature
User impact if declined: broken double click behavior in the Library.
Testing completed (on m-c, etc.): tbd.
Risk to taking this patch (and alternatives if risky): limited to the ui action involved.
String or UUID changes made by this patch: none.

Comment 24

[Marco Bonardo [:mak]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d135f2c0d621

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3dcd159e6cb5

Comment 26

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Windows NT 6.2; rv:20.0) Gecko/20130204 Firefox/20.0
Build ID: 20130204042019
Mozilla/5.0 (Windows NT 6.2; rv:21.0) Gecko/20130204 Firefox/21.0
Build ID: 20130204030941

Verified as fixed in latest Nightly and Aurora.