[regression] Urls in the history panel are missing

VERIFIED FIXED

Status

VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

Trunk
x86
Linux
Bug Flags:
in-litmus +

Details

Attachments

(1 attachment, 2 obsolete attachments)

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
Attachment #488111 - Flags: review?(mbrubeck)
Attachment #488111 - Flags: review?(mbrubeck) → review+
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.
Attachment #488111 - Flags: review-
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!
Attachment #488111 - Flags: review+
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?
Assignee: nobody → 21
Attachment #488111 - Attachment is obsolete: true
Attachment #488224 - Flags: review?(mbrubeck)
Attachment #488224 - Flags: review?(mark.finkle)
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.
Attachment #488224 - Flags: review?(mbrubeck)
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...
Attachment #488224 - Attachment is obsolete: true
Attachment #488248 - Flags: review?(mbrubeck)
Attachment #488248 - Flags: review?(mark.finkle)
Attachment #488224 - Flags: review?(mark.finkle)
Summary: [regression] Site menu should be hidden when clicking on the awesome bar → [regression] Urls in the history panel are missing
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?
Attachment #488248 - Flags: review?(mbrubeck) → review+
Comment on attachment 488248 [details] [diff] [review]
Patch v0.4

Yes, add the "subtitle" fix to remote-tabs too
Attachment #488248 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mobile-browser/rev/160b489b6c6a
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
verified FIXED on build:

Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101105 Namoroka/4.0b8pre Fennec/4.0b2pre
Status: RESOLVED → VERIFIED
Flags: in-litmus?

Updated

8 years ago
Flags: in-litmus? → in-litmus?(aaron.train)
You need to log in before you can comment on or make changes to this bug.