501413 - In a right-to-left localization, clicking any url in the bookmark sidebar doesn't work

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[offir]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; he; rv:1.9.1) Gecko/20090624 Firefox/3.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; he; rv:1.9.1) Gecko/20090624 Firefox/3.5

i use firefox 3.5 in hebrew and when i click any url in the bookmark sidebar,there isn't any response

the only way to open a website,is to right click,and choose open

Reproducible: Always

Steps to Reproduce:
1.click the bookmark icon in the tooblar to open the sidebar bookmark
2.click any url in bookmark
3.
Actual Results:  
there isn't any response when clicking any url

Expected Results:  
open the url that i clicked

Comment 1

[Tyler Downer [He/Him]]

Does it happen in Firefox Safe Mode?

Comment 2

[Simon Montagu :smontagu]

I can confirm this bug in the Hebrew localization. However, if I click way off in the right margin in the bookmarks sidebar, the links open as expected.

Ehsan, is this a problem with click targetting in the right-to-left tree?

Comment 3

[Simon Montagu :smontagu]

So maybe there is an underlying rtl bug, but why is it that right click and mouse-over work on the whole line but left click only works on the link text? It's a strange inconsistency.

Comment 4

[Tomer Cohen :tomer]

I can reproduce on Linux.

Platform -> All
keyword -> rtl

Comment 5

[(no longer active)]

(In reply to comment #2)
> I can confirm this bug in the Hebrew localization. However, if I click way off
> in the right margin in the bookmarks sidebar, the links open as expected.

I can reproduce the same in the Persian localization.  The right margin which works is the tree line and twisty area.  Clicking on the icon, text or the left side margin doesn't work.

> Ehsan, is this a problem with click targetting in the right-to-left tree?

That's what I first thought, but it's not the case.  The problem is with this code: <http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/sidebarUtils.js#53>.  This code incorrectly assumes that the image for each item is always at its left side, whereas in RTL trees it's at the right side.  This causes clicks anywhere but the right side of the image have no effect.

Patch forthcoming.

Comment 6

[(no longer active)]

Attachment: Patch (v1)

Simple patch; just reverse the logic in RTL mode.

Comment 7

[(no longer active)]

This bug makes the bookmarks and history sidebars look broken for our RTL localizations.  Given the simplicity of the fix, and that a major piece of secondary UI is broken, I'm requesting that this blocks 1.9.1.1.  I should note however that the context menu workaround works in broken RTL builds, but I'm not sure how discoverable that is for users, and whether if users try it after seeing that normal clicks seemingly do nothing.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 386108 [details] [diff] [review]
Patch (v1)

if i read this correctly the behavior in the 2 cases is different, in non rtl mode clicking the icon visits the page, in rtl mode clicking the icon does not visit the page. Imo they should be consistent.

Comment 9

[(no longer active)]

(In reply to comment #8)
> (From update of attachment 386108 [details] [diff] [review])
> if i read this correctly the behavior in the 2 cases is different, in non rtl
> mode clicking the icon visits the page, in rtl mode clicking the icon does not
> visit the page. Imo they should be consistent.

No.  getCoordsForCellItem returns x in logical direction (that is, from the right side in RTL mode), not in physical direction (that is, from the left side in both RTL and LTR modes).  Therefore, the code makes sure that the icon is clickable in both modes.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 386108 [details] [diff] [review]
Patch (v1)

oh ok, i was not aware of that wise behavior. Could you please add a comment about that before getCoordsForCellItem?

r=me with that

Comment 11

[Marco Bonardo [:mak]]

oh and since isRTL is only used inside if (aGutterSelect), you should get it there.

Comment 12

[(no longer active)]

Attachment: For check-in

The patch to be checked in, taking comments 10 and 11 into consideration.

Comment 13

[(no longer active)]

Can someone please check this in, because I can't use the ssh service for the time being.

Comment 14

[Simon Montagu :smontagu]

http://hg.mozilla.org/mozilla-central/rev/6c06e841a7d5

Thanks for the fast fix, Ehsan!

Comment 15

[Tomer Cohen :tomer]

Issue is reproducible on the History sidebar (ctrl-h), and maybe on other sidebars (such as the all-in-one-sidebar addon) as well. 

Didn't checked a nightly yet but Fx3.5 - Does the nightlies have this patch already?

Comment 16

[(no longer active)]

(In reply to comment #15)
> Issue is reproducible on the History sidebar (ctrl-h), and maybe on other
> sidebars (such as the all-in-one-sidebar addon) as well. 

It should happen in all places sidebars (History and Bookmarks).  Sidebars by extensions should not be affected unless they use chrome://browser/content/bookmarks/sidebarUtils.js.

> Didn't checked a nightly yet but Fx3.5 - Does the nightlies have this patch
> already?

The nightlies have this patch.  I hope that this gets into Fx3.5.1 if approved by the drivers.

Comment 18

[Simon Montagu :smontagu]

Ehsan, will you reqest approval1.9.1.1 for the patch?

Comment 19

[(no longer active)]

Comment on attachment 386228 [details] [diff] [review]
For check-in

(In reply to comment #18)
> Ehsan, will you reqest approval1.9.1.1 for the patch?

Definitely.  Thanks for the reminder.

Comment 20

[Simon Montagu :smontagu]

I hope the patch gets approved for 1.9.1.1 in spite of this not being marked as a blocker. Perhaps this seems like a small problem in uncommonly-used UI, but apparently regular users use the bookmarks sidebar more than we realize: there have been numerous reports of this problem in the community forum in Israel and IRC #mozilla.il since the release of Firefox 3.5.

Comment 21

[(no longer active)]

Beltzner: please see comment 20.

Comment 22

[Tomer Cohen :tomer]

Ehsan, can you request approval1.9.1.2 please?

Comment 23

[Tomer Cohen :tomer]

I heard some rumors that this issue is reproducible using all in one sidebar addon (see comment 15). I've asked on our local forum if someone is able to reproduce, and will make sure our fix won't create any regressions on such addons.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 386228 [details] [diff] [review]
For check-in

a1912=beltzner with the test you obviously meant to attach but didn't because you knew that I just would assume you to be the type of person to know that you had to have a test for this :)

Comment 25

[(no longer active)]

Attachment: Test

(In reply to comment #24)
> (From update of attachment 386228 [details] [diff] [review])
> a1912=beltzner with the test you obviously meant to attach but didn't because
> you knew that I just would assume you to be the type of person to know that you
> had to have a test for this :)

Well, you know me too well!  ;-)

Here is the test.  As a bonus, this test makes sure that items in both the history sidebar and the bookmarks sidebar are clickable in *both* RTL and LTR mode.

Comment 26

[Marco Bonardo [:mak]]

Comment on attachment 390730 [details] [diff] [review]
Test

as a general coding style for comments please uppercase first and end them with a dot.

>diff --git a/browser/components/places/tests/browser/browser_sidebarpanels_click.js b/browser/components/places/tests/browser/browser_sidebarpanels_click.js

>+
>+// This test makes sure that the items in the bookmarks and history sidebar
>+// panels are clickable.

... in both LTR and RTL modes.

>+
>+function test() {
>+  // initialization
>+  let ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].
>+           getService(Ci.nsIWindowWatcher);
>+  let bs = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+           getService(Ci.nsINavBookmarksService);

bs = PlacesUtils.bookmarks

>+  let history = PlacesUtils.history;

hs, to be consistent with bs

>+  let sidebarBox = document.getElementById("sidebar-box");
>+  let sidebar = document.getElementById("sidebar");
>+  waitForExplicitFinish();
>+
>+  // if a sidebar is already open, close it.
>+  if (!sidebarBox.hidden)
>+    toggleSidebar();

is this really needed? later you can call toggleSidebar(sidebarName, true); that will ensure the sidebar you need is open.

>+  let bookmarksPanelTest = {

why not using an array of tests and simplify the thing with a run_next_test function that will check if are there still tests to run?
That will allow to add new tests in future and make the test more maintainable.

>+    init: function() {
>+      // Add a bookmark to the Unfiled Bookmarks folder
>+      return bs.insertBookmark(bs.unfiledBookmarksFolder,
>+                               PlacesUtils._uri(TEST_URL),
>+                               bs.DEFAULT_INDEX, "test");

you can save the id in a local private property for this test, without having to pass it out and pass it back later.

>+    },
>+    prepare: function() {
>+    },
>+    selectNode: function(tree, testID) {
>+      tree.selectItems([testID]);

so here you can use the local this._itemId property

>+    },
>+    cleanup: function() {
>+      bs.removeFolderChildren(bs.unfiledBookmarksFolder);
>+    },
>+    sidebarName: "viewBookmarksSidebar",
>+    treeName: "bookmarks-view",
>+    type: "bookmark"

i can't see a clear reason to define a type, unless you hold elements ids in an array and you get them like let (sidebarName,treeName) = elementsIds[test.type];
rather i'd simply add a desc property with a description of what the test is going to do, and info() it when running the test (the run_next_test method could take care of that)
and element ids should probably live in BOOKMARKS_SIDEBAR_ID and BOOKMARKS_SIDEBAR_TREE_ID const above everything.

>+  };
>+  let historyPanelTest = {

ditto

>+    init: function() {
>+      // Add a history entry
>+      this.cleanup();
>+      return history.addVisit(PlacesUtils._uri(TEST_URL), Date.now() * 1000,
>+                              null, PlacesUtils.history.TRANSITION_TYPED, false, 0);

PlacesUtils.history is useless since you defined an abbr

>+    },
>+    prepare: function() {
>+      sidebar.contentDocument.getElementById("byvisited").doCommand();
>+    },
>+    selectNode: function(tree) {
>+      tree.selectNode(tree.view.nodeForTreeIndex(0));
>+      is(tree.selectedNode.uri, TEST_URL, "The correct visit has been selected");
>+      is(tree.selectedNode.itemId, -1, "The selected node is not bookmarked");
>+    },
>+    cleanup: function() {
>+      history.QueryInterface(Ci.nsIBrowserHistory)
>+             .removeAllPages();
>+    },
>+    sidebarName: "viewHistorySidebar",
>+    treeName: "historyTree",
>+    type: "history"

ditto

>+  };
>+
>+  let currentTest;
>+
>+  function testPlacesPanel(preFunc, postFunc) {
>+    let testID = currentTest.init();
>+    ok(testID > 0, "Test " + currentTest.type + " successfully added");

i know this is something we do in many tests, but actually is completely useless, we won't return -1, if adding the visit or bookmark will fail it will simply throw.

>+    sidebar.addEventListener("load", function() {
>+      sidebar.removeEventListener("load", arguments.callee, true);
>+
>+      let doc = sidebar.contentDocument;
>+      let tree = doc.getElementById(currentTest.treeName);
>+      let tbo = tree.treeBoxObject;
>+
>+      executeSoon(function() {
>+        currentTest.prepare();
>+        if (preFunc)
>+          preFunc();
>+
>+        let observer = {
>+          observe: function(aSubject, aTopic, aData) {
>+            if (aTopic === "domwindowopened") {
>+              ww.unregisterNotification(this);
>+              let alertDialog = aSubject.QueryInterface(Ci.nsIDOMWindow);
>+              alertDialog.addEventListener("load", function() {
>+                alertDialog.removeEventListener("load", arguments.callee, false);
>+                ok(true, "alert dialog observed as expected");
>+                executeSoon(function() {
>+                  alertDialog.close();
>+                  toggleSidebar();

use toggleSidebar(sidebarName, true/false);

>+                  currentTest.cleanup();
>+                  postFunc();
>+                });
>+              }, false);
>+            }
>+          }
>+        };
>+        ww.registerNotification(observer);
>+
>+        // Select the inserted places item
>+        currentTest.selectNode(tree, testID);
>+        is(tbo.view.selection.count, 1,
>+           "The test " + currentTest.type + " should be successfully selected");
>+        // Get its row ID
>+        let min = {}, max = {};
>+        tbo.view.selection.getRangeAt(0, min, max);
>+        let rowID = min.value;
>+        tbo.ensureRowIsVisible(rowID);
>+
>+        // Calculate the click coordinates
>+        let x = {}, y = {}, width = {}, height = {};
>+        tbo.getCoordsForCellItem(rowID, tree.columns[0], "text",
>+                                 x, y, width, height);
>+        x = x.value + width.value / 2;
>+        y = y.value + height.value / 2;
>+        // Simulate the click
>+        EventUtils.synthesizeMouse(tree.body, x, y, {}, doc.defaultView);
>+        // Now, wait for the domwindowopened observer to catch the alert dialog.
>+        // If something goes wrong, the test will time out at this stage.
>+        // Note that for the history sidebar, the URL itself is not opened,
>+        // and Places will show the load-js-data-url-error prompt as an alert
>+        // box, which means that the click actually worked, so it's good enough
>+        // for the purpose of this test.
>+      });
>+    }, true);
>+    toggleSidebar(currentTest.sidebarName);

ditto

>+  }
>+
>+  function makeSidebarRTL() {
>+    document.getElementById("sidebar")
>+            .contentDocument
>+            .documentElement
>+            .style.direction = "rtl";
>+  }

could be that in future tests will be run with localization enabled, so maybe this will originally be run in RTL and this should instead toggle it to LTR... make this something like changeSidebarDirection("rtl") and changeSidebarDirection("ltr"), and call both so you ensure to test both in all cases.

>+
>+  currentTest = bookmarksPanelTest;
>+  ok(true, "Testing the bookmarks sidebar");
>+
>+  testPlacesPanel(null, function() {
>+    testPlacesPanel(function() {

the tests array solution would make this simpler to read and maintain and break the main thread loop (using executeSoon)

>+      // Test the bookmarks panel in RTL mode
>+      makeSidebarRTL();
>+      ok(true, "Testing the bookmarks sidebar in RTL mode");
>+    }, function() {
>+      currentTest = historyPanelTest;
>+      ok(true, "Testing the history sidebar");
>+      testPlacesPanel(null, function() {
>+        testPlacesPanel(function() {
>+          // Test the history panel in RTL mode
>+          makeSidebarRTL();
>+          ok(true, "Testing the history sidebar in RTL mode");
>+        }, finish);
>+      });
>+    });
>+  });
>+}

Comment 27

[(no longer active)]

Attachment: Test (v2)

(In reply to comment #26)
> >+  // if a sidebar is already open, close it.
> >+  if (!sidebarBox.hidden)
> >+    toggleSidebar();
> 
> is this really needed? later you can call toggleSidebar(sidebarName, true);
> that will ensure the sidebar you need is open.

Yes, it's needed, because I rely on the load event of the sidebars, and it won't fire if the sidebar is already open.

> >+                  toggleSidebar();
> 
> use toggleSidebar(sidebarName, true/false);

There is no need to pass the forceOpen parameter, since the sidebar is already open and toggleSidebar simply closes it.

> >+
> >+  currentTest = bookmarksPanelTest;
> >+  ok(true, "Testing the bookmarks sidebar");
> >+
> >+  testPlacesPanel(null, function() {
> >+    testPlacesPanel(function() {
> 
> the tests array solution would make this simpler to read and maintain and break
> the main thread loop (using executeSoon)

I'm not sure why you suggest using executeSoon, but I added it anyway.


I fixed all of the other comments as well.

Comment 28

[Marco Bonardo [:mak]]

Comment on attachment 391072 [details] [diff] [review]
Test (v2)

>diff --git a/browser/components/places/tests/browser/browser_sidebarpanels_click.js b/browser/components/places/tests/browser/browser_sidebarpanels_click.js
>+
>+  // If a sidebar is already open, close it.
>+  if (!sidebarBox.hidden)
>+    toggleSidebar();

could you add an info() saying the the sidebar was wrongly found open? since this should not happen, so we can maybe detect if previous tests are not cleaning up correctly.

>+
>+  const TEST_URL = "javascript:alert(\"test\");";
>+
>+  let tests = [];
>+  tests.push({
>+    _itemID: null,
>+    init: function() {
>+      // Add a bookmark to the Unfiled Bookmarks folder.
>+      this._itemID = bs.insertBookmark(bs.unfiledBookmarksFolder,
>+                                       PlacesUtils._uri(TEST_URL),
>+                                       bs.DEFAULT_INDEX, "test");
>+    },
>+    prepare: function() {
>+    },
>+    selectNode: function(tree) {
>+      tree.selectItems([this._itemID]);
>+    },
>+    cleanup: function() {
>+      bs.removeFolderChildren(bs.unfiledBookmarksFolder);
>+    },
>+    sidebarName: BOOKMARKS_SIDEBAR_ID,
>+    treeName: BOOKMARKS_SIDEBAR_TREE_ID,
>+    desc: "Bookmarks sidebar test"
>+  });
>+  tests.push({

add a newline between tests please so they are more easily detectable with eyes

>+  function changeSidebarDirection(direction) {

aDirection please

>+    document.getElementById("sidebar")
>+            .contentDocument
>+            .documentElement
>+            .style.direction = direction;
>+  }
>+
>+  function runNextTest() {
>+    ++currentTestIndex;
>+    if (currentTestIndex == tests.length)
>+      finish();
>+    else {
>+      currentTest = tests[currentTestIndex];

you can simplify this using currentTest = tests.shift(); and check tests.length > 0

Comment 29

[(no longer active)]

Tests landed as <http://hg.mozilla.org/mozilla-central/rev/de81fd113028>

Comment 30

[(no longer active)]

... and corrected a last minute mistake: <http://hg.mozilla.org/mozilla-central/rev/4416e857d9a7>

Comment 31

[Marco Bonardo [:mak]]

the test could have caused a random failure in browser/components/places/tests/perf/browser_ui_bookmarks_sidebar.js
Not sure about this, but seems possible since both are using the sidebars

Comment 32

[Marco Bonardo [:mak]]

and i can't find previous failures of that test before

Comment 33

[Marco Bonardo [:mak]]

filed Bug 507172 about it, seeing if i can reproduce locally.

Comment 34

[Samuel Sidler (old account; do not CC)]

Comment on attachment 386228 [details] [diff] [review]
For check-in

This didn't land in time for 1.9.1.2.

Comment 35

[Tomer Cohen :tomer]

(In reply to comment #34)
> This didn't land in time for 1.9.1.2.
This is the 2nd time we miss the release with this commit even as we had (thanks to Ehsan) the fix available at 3.5.0 release day. I have seen this issue raised in our local forum too much times, and users are still complaining and angry as the fix was not included in 3.5.1 release. 

Please make sure it is commited to 3.5.x branch as soon as possible.

Comment 36

[(no longer active)]

(In reply to comment #33)
> filed Bug 507172 about it, seeing if i can reproduce locally.

Could you reproduce this locally?  I tried 100 consecutive runs, without a single failure...

Comment 37

[(no longer active)]

(In reply to comment #35)
> (In reply to comment #34)
> > This didn't land in time for 1.9.1.2.
> This is the 2nd time we miss the release with this commit even as we had
> (thanks to Ehsan) the fix available at 3.5.0 release day. I have seen this
> issue raised in our local forum too much times, and users are still complaining
> and angry as the fix was not included in 3.5.1 release. 

I'm sure that by now the drivers are aware of the impact of this bug.  But shipping a patch to a stability release branch takes several steps, including the time required for the patch to bake on trunk.  And the fact that I was half offline in the past few weeks and failed to submit an automated unit test along with the original patch also contributed to the delay here.  Anyway, there only remains one more step now: getting the patch approved.  :-)

> Please make sure it is commited to 3.5.x branch as soon as possible.

I'm sure that 1.9.1.3 will ship with this fixed, unless something unexpected happens (like for example the release schedule acceleration which happened for 1.9.1.1.)

Comment 38

[Marco Bonardo [:mak]]

(In reply to comment #36)
> (In reply to comment #33)
> > filed Bug 507172 about it, seeing if i can reproduce locally.
> 
> Could you reproduce this locally?  I tried 100 consecutive runs, without a
> single failure...

no i cannot reproduce locally, i suspect cycle collector, btw i actually disabled the failing test in bug 507172... there are other failures recently that i cannot reproduce nor explain, so hard to tell what's up.

Comment 39

[Tomer Cohen :tomer]

This bug is very important for RTL languages, please fix it for Firefox 3.5 users as soon as possible. Thanks!

here are some links for [some of the] users complaining about this issue:
http://mozilla.org.il/board/viewtopic.php?t=7760
http://mozilla.org.il/board/viewtopic.php?t=7722
http://mozilla.org.il/board/viewtopic.php?t=7753
http://firefox.exxile.net/forum/viewtopic.php?t=8773

Comment 40

[Samuel Sidler (old account; do not CC)]

Comment on attachment 386228 [details] [diff] [review]
For check-in

Approved for 1.9.1.4. a=ss for release-drivers

Comment 41

[(no longer active)]

All three patches pushed cumulatively on 1.9.1:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5cc045a90d95

Comment 42

[Al Billings [:abillings - ex-MoCo]]

Verified that this is fixed on 1.9.1.4 with Mozilla/5.0 (X11; U; Linux i686; he; rv:1.9.1.4pre) Gecko/20090921 Shiretoko/3.5.4pre.