Doorhanger popup can appear on top of awesomescreen in editing mode

VERIFIED FIXED in Firefox 26

Status

()

VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Trunk
Firefox 26
ARM
Android
Points:
---

Firefox Tracking Flags

(fennec26+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 794203 [details]
Screenshot_2013-08-22-15-23-14.png

See bug 906041 comment 6 for STR.

If we avoid showing the notification while in editing mode, we'll want to make sure to show it after exiting editing mode. I'm worried we might end up with some tricky logic here :/
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → 26+
(Assignee)

Comment 1

5 years ago
Created attachment 800444 [details] [diff] [review]
patch

I don't love that this patch sprinkles mDoorHangerPopup.enable() calls all over the place, but that's a by-product of the fact that there are multiple calls to mBrowserToolbar.cancelEdit/commitEdit. I suppose it would be nicer if BrowserToolbar could be handling these things, but it doesn't know about DoorHangerPopup.

I'm open to suggestions if anyone has ideas about how to make this better, but it does get the job done.
Attachment #800444 - Flags: review?(bnicholson)
I seems like BrowserToolbar should have a callback interface for starting and ending editing. Then BrowserApp could implement the interface, and pass "this" to BrowserToolbar.

BrowsserToolbar would call the onStartEditing and onStopEditing(/*cancel or commit*/) giving BrowserApp know locations where we would enable and disable the doorhanger.
I agree we should factor this out into some kind of interface. We already have hideBrowserSearch() at every call to cancelEdit()/commitEdit(), so an interface would be a nice way to get rid of this growing code duplication.

An alternative (but similar) approach you could use would be to pass the implemented callbacks directly to commitEdit()/cancelEdit().
(Assignee)

Comment 4

5 years ago
I just noticed we already have these listeners:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#451

However, we can't just stick our mDoorHangerPopup.enable/disable calls in there because there are other places we call enterEditingMode() and dismissEditingMode().

Maybe we need to reorganize some of this logic to put BrowserToolbar more in charge of the state of things, so that these listeners are always called.
Comment on attachment 800444 [details] [diff] [review]
patch

Removing r? for listener changes. Hopefully using interfaces will be more straightforward this time!
Attachment #800444 - Flags: review?(bnicholson)
(Assignee)

Comment 6

5 years ago
Created attachment 803857 [details] [diff] [review]
patch v2

To keep this patch well-scoped, I only put the doorhanger stuff in the listeners right now, but we could do some more refactoring on top of this.

One concern I have is that maybe I should have put the onStartEditing/onStopEditing calls lower down in startEditing/stopEditing, below the animation logic. However, to do that, I'd have to refactor those methods a bit to avoid the early returns.
Attachment #800444 - Attachment is obsolete: true
Attachment #803857 - Flags: review?(bnicholson)
Attachment #803857 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/33fbbf1340d9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.