Closed
Bug 609516
Opened 12 years ago
Closed 12 years ago
[regression] Urls in the history panel are missing
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: vingtetun, Assigned: vingtetun)
Details
Attachments
(1 file, 2 obsolete files)
2.41 KB,
patch
|
mbrubeck
:
review+
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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)
Updated•12 years ago
|
Attachment #488111 -
Flags: review?(mbrubeck) → review+
Comment 1•12 years ago
|
||
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 2•12 years ago
|
||
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+
Assignee | ||
Comment 3•12 years ago
|
||
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 4•12 years ago
|
||
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)
Assignee | ||
Comment 5•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Summary: [regression] Site menu should be hidden when clicking on the awesome bar → [regression] Urls in the history panel are missing
Comment 6•12 years ago
|
||
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 7•12 years ago
|
||
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+
Assignee | ||
Comment 8•12 years ago
|
||
http://hg.mozilla.org/mobile-browser/rev/160b489b6c6a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 9•12 years ago
|
||
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•12 years ago
|
Flags: in-litmus? → in-litmus?(aaron.train)
Comment 10•12 years ago
|
||
https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=13816
Flags: in-litmus?(aaron.train) → in-litmus+
You need to log in
before you can comment on or make changes to this bug.
Description
•