1010771 - Implement overflow / options menu

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

Comment 1

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/19432

We should have integration tests for this, but thats currently impossible due to https://bugzilla.mozilla.org/show_bug.cgi?id=1009855 which I am going to start working on

There are some event tracking issues discussed on irc that I want to clean up, but didnt want to overload this patch

Will need to file bugs for additional overflow menu items (open new tab)

Comment 2

[Dale Harvey (:daleharvey)]

The navigation part of this was merged from https://bugzilla.mozilla.org/show_bug.cgi?id=941174, it should be a trivial merge when thats read though

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8425411 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/19432

See github comments, feel free to leave comment here or ping me online.

Comment 4

[Dale Harvey (:daleharvey)]

Comment on attachment 8425411 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/19432

Addressed the nits, I have called it |showDefaultContextMenu| as I see this as a context menu that is analogous to clicking on empty space on a page, the current context menu works when clicking on recognised elements on a page

Also a bit late, will remember for the next bug, but 

Specs https://mozilla.box.com/s/w1k1k90875m7cofgx3m8
Flow https://mozilla.box.com/s/1cfolb0hxu5w6yzxs8cv

are the specs

Comment 5

[Ben Francis [:benfrancis]]

Comment on attachment 8425411 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/19432

This code looks generally OK.

I really didn't imagine the overflow menu as the same thing as the context menu, I saw them as completely separate. But as long as it can be implemented fully to spec this way I guess that's OK.

This implementation clearly doesn't match the visual specs. Perhaps you are assuming some of this will get fixed system-wide? But both the interaction spec [1] and visual specs you reference show icons for the menu items, which isn't implemented here. I think at least that needs implementing before this bug can be resolved as fixed.

I'd prefer not to half-implement the bookmark to homescreen feature in this bug without setting an icon, but if that's the last thing to do then I wouldn't block landing on it as it's outside the scope of this bug.

1. https://mozilla.app.box.com/files/0/f/1399872384/1/f_16451967037

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8425411 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/19432

r+ if github comments are addressed.

Comment 7

[Dale Harvey (:daleharvey)]

The visuals are system wide, I opened a new bug to track them - https://bugzilla.mozilla.org/show_bug.cgi?id=1014480

Having the icons doesnt make sense until we implement the visual refresh, but removing all the functionality until we have the refresh does not make sense, there are various things that still need to be implemented (forward, open in new tab), this bug was for the initial implementation of the overflow menu, we can / mostly have follow up bugs for individual functionality

Addressed Alives review comments

Landed in: https://github.com/mozilla-b2g/gaia/commit/6041132e9635a14764fe81b6accb522b8d2f82c5

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d19aa935c1af

Comment 9

[Dale Harvey (:daleharvey)]

Apologies, I had addressed bens comments about the icon (good catch), and forgot to ammend the commit, its a minor addition so r=me and pushed it 

https://github.com/mozilla-b2g/gaia/commit/ed96d8aea83846002958bdfaafcfcdca816a3e58

Comment 10

[Dale Harvey (:daleharvey)]

And again, realised my original patch wouldnt work for pages missing favicons, wont do that again, sorry

https://github.com/mozilla-b2g/gaia/commit/9729730477d10d345002470fd855d56d9d30ea15

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/536ac64d0d40
https://hg.mozilla.org/mozilla-central/rev/d349e5704140