609516 - [regression] Urls in the history panel are missing

Status: VERIFIED FIXED

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Created attachment 488111 [details] [diff] [review]
Patch

The patch remove the toolbar from the authorize elements list for the site menu (not sure why it was there), it also move the hidePanel call inside the showAutocomplete function close to the _hidePopup call.

The patch also include a small fix for a regression on the history list

Comment 1

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

Comment on attachment 488111 [details] [diff] [review]
Patch

Hold on a minute:
* Elements.toolbarContainer was added to the site menu pushPopup because of an Android problem. See bug 567044. make sure we don't regress it
* Your history list fix is not good enough. You replace the setAttribute("url", ...) with setAttribute("subtitle", ...). Great, now the URL is visible in the row, but opening the link is broken. AwesomePanel.openLink expects a "url" attribute. We must use both "url" and "subtitle". Oh and remote-tabs needs the same fix.

Comment 2

[Matt Brubeck (:mbrubeck)]

Comment on attachment 488111 [details] [diff] [review]
Patch

(In reply to comment #1)
> * Elements.toolbarContainer was added to the site menu pushPopup because of an
> Android problem. See bug 567044. make sure we don't regress it

Oops, looks like this does regress bug 567044.  Sorry for missing those!

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Created attachment 488224 [details] [diff] [review]
Patch v0.2

I don't understand how this could regress the urlbar tapping, in fact I don't really understand the patch since it adds the main toolbar area to the tapping area where we should not dismiss the popup...
In all cases this patch seems to work on device and I have addressed the subtitle case.
Do I missed something?

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 488224 [details] [diff] [review]
Patch v0.2

(In reply to comment #3)
> I don't understand how this could regress the urlbar tapping, in fact I don't
> really understand the patch since it adds the main toolbar area to the tapping
> area where we should not dismiss the popup...

Steps to reproduce:
1) Scroll down so the urlbar is hidden.
2) Press the "Menu" key to display the menu and urlbar.
3) Tap the urlbar.

Expected results: Site menu closes, awesomescreen opens.
Actual results (with this patch): Site menu closes, awesomescreen does not open.

It also breaks tapping on the "reload" button if it's displayed by the Menu key.

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Created attachment 488248 [details] [diff] [review]
Patch v0.4

Thanks for the str, for some reasons I don't see the regression on today's build :s

My best option is probably a bugmorph to address the history case...

Comment 6

[Matt Brubeck (:mbrubeck)]

Comment on attachment 488248 [details] [diff] [review]
Patch v0.4

Looks good to me.  Can you also add the "subtitle" fix to the Desktop (remotetabs) list?

Comment 7

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

Comment on attachment 488248 [details] [diff] [review]
Patch v0.4

Yes, add the "subtitle" fix to remote-tabs too

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mobile-browser/rev/160b489b6c6a

Comment 9

[Aakash Desai [:aakashd]]

verified FIXED on build:

Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101105 Namoroka/4.0b8pre Fennec/4.0b2pre

Comment 10

[Aaron Train [:aaronmt]]

https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=13816