480950 - unable to edit bookmarks from the manage mode

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Bookmarks, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

in fennec:
1) add a bookmark.  
2) click the bookmark icon next to the url bar
3) click manage button
4) click "edit" next to your bookmark
5) observe that it switches to edit mode, then away from it immediately

the net result is you cannot edit your bookmark.  This fails with and without the vkb.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

Not sure if this is the prefect fix, but it fixes this bug. In the ChromeInputHandler._onMouseDown we try to find the scrollboxobject of the clicked element. If we find a scrollable element we start some event redispacthing.

This patch does two things:
1. If the clicked element is a nsIDOMXULButtonElement, we bail out.
2. We use the event.originalTarget to locate the clicked element. The event.target wasn't returning buttons embedded in XBL.

Comment 2

[Ben Combee (:bcombee)]

Comment on attachment 364911 [details] [diff] [review]
patch

Looks good.  Q's were answered on IRC:

08:06 < bcombee> ack... the change seems OK, but we probably need something more
                 general
08:06 < bcombee> this will work for now
08:06 < bcombee> how do the input fields work after you hit edit?
08:06 < mfinkle> bcombee: nsIDOMXULButtonElement should give us support for"
                 button, toolbarbutton, checkbox and maybe radiobutton
08:07 < bcombee> also, the change to use originalTarget in the redispatch... why?
08:07 < mfinkle> without the check, we were re-dispatching the mousedown to the
                 button, which also handled the "click", I think
08:07 < mfinkle> so the button was "clicked" twice
08:08 < mfinkle> the event.originalTarget change was needed for elements embedded
                 in XBL
08:08 < bcombee> cool
08:08 < mfinkle> event.target == placelist
08:08 < mfinkle> event.originalTarget == button
08:08 < bcombee> OK, I'll r+ in a moment

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: better alternative patch

This patch is better, IMO, because it ignores the "click" events, unless we regenerate mouse events. This keeps the buttons from firing twice (initial "click" is ignored) and allows us to pan even if a button was under mousedown (first patch would not do that)

Comment 4

[Ben Combee (:bcombee)]

Comment on attachment 364947 [details] [diff] [review]
better alternative patch

I ilke it.  I might change _ignoreClick to _ignoreClickOnce to make it clear that it's a one-shot property, but the logic seems fine.

We don't need this for the content handler, I assume.  I think the browser surface will only get the one click it needs.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

http://hg.mozilla.org/mobile-browser/rev/4f72f60cdb15

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

we have redone the bookmarks.

verified with 20091001 1.9.2 b4 on my n810

Comment 7

[Aakash Desai [:aakashd]]

Litmus testcase: https://litmus.mozilla.org/show_test.cgi?id=7640

Covers the overall BFT breadth of testing for this bug.