1128390 - Removal confirm dialog and download failure drawer for bug 1029951

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

+++ This bug was initially created as a clone of Bug #1029951 +++

See
https://bug983043.bugzilla.mozilla.org/attachment.cgi?id=8522035#12

The user would need to see a confirmation dialog before the layout is actually uninstalled.

Also it would need to see a download failed banner.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Wilson, I am getting the following error inside gaia-toast with my patch.

W/Built-in Keyboard( 1921): [JavaScript Error: "TypeError: component is undefined" {file: "app://keyboard.gaiamobile.org/shared/elements/gaia-toast/gaia-toast.js" line: 15}]

I don't know why the same

var component = require('gaia-component');

would fail on gaia-toast.js but works in gaia-header.js. Would you mind shed some light here?

https://github.com/mozilla-b2g/gaia/compare/master...timdream:keyboard-dyn-dialogs

Comment 2

[Wilson Page [:wilsonpage]]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #1)
> Wilson, I am getting the following error inside gaia-toast with my patch.
> 
> W/Built-in Keyboard( 1921): [JavaScript Error: "TypeError: component is
> undefined" {file:
> "app://keyboard.gaiamobile.org/shared/elements/gaia-toast/gaia-toast.js"
> line: 15}]
> 
> I don't know why the same
> 
> var component = require('gaia-component');
> 
> would fail on gaia-toast.js but works in gaia-header.js. Would you mind shed
> some light here?
> 
> https://github.com/mozilla-b2g/gaia/compare/master...timdream:keyboard-dyn-
> dialogs

You need to make sure gaia-component.js [1] is loaded before gaia-toast.js.

[1] https://github.com/gaia-components/gaia-component

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Wilson Page [:wilsonpage] from comment #2)
> You need to make sure gaia-component.js [1] is loaded before gaia-toast.js.
> 
> [1] https://github.com/gaia-components/gaia-component

Yeah it works, but after testing I realized <gaia-toast> does not do what it intend to do sometimes.

I will experiment more maybe submit a patch to the component.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Wilson,

Given the hide animation can be interrupted by the new show() call, you would need to clean up that too.

https://github.com/gaia-components/gaia-toast/pull/1

I also wonder if it's possible for the script to never receive animationend event, if the animation is interrupted (say, if the parent element is set with display: none in the middle of the animation.). You might need to use a setTimeout to trigger onAnimateOutEnd instead of the event just to safe guard that.

I've also realized the current component impl does not attempt to hide internal properties such as |els| or |onAnimateOutEnd|. They remain accessible from the DOM. I think to ensure encapsulation, we should hide them. If there aren't private instance created when the DOM element is created, we should try to hide these information with WeakMap.

https://gist.github.com/timdream/379bdd1d26d807556bba

What do you think?

Comment 5

[Wilson Page [:wilsonpage]]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #4)
> Wilson,
> 
> Given the hide animation can be interrupted by the new show() call, you
> would need to clean up that too.
> 
> https://github.com/gaia-components/gaia-toast/pull/1
> 
> I also wonder if it's possible for the script to never receive animationend
> event, if the animation is interrupted (say, if the parent element is set
> with display: none in the middle of the animation.). You might need to use a
> setTimeout to trigger onAnimateOutEnd instead of the event just to safe
> guard that.
> 
> I've also realized the current component impl does not attempt to hide
> internal properties such as |els| or |onAnimateOutEnd|. They remain
> accessible from the DOM. I think to ensure encapsulation, we should hide
> them. If there aren't private instance created when the DOM element is
> created, we should try to hide these information with WeakMap.
> 
> https://gist.github.com/timdream/379bdd1d26d807556bba
> 
> What do you think?

Yes I think we need a pattern to follow for this to hide the 'gut'. In some libraries I've been doing something simpler like this [1], but that may not work as well with inheritance. Although I'd favour the simplest solution possible.

[1] https://gist.github.com/wilsonpage/ecfcd25203cc7ca91851

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] timdream:keyboard-dyn-dialogs > mozilla-b2g:master

Comment 7

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8592147 [details] [review]
[gaia] timdream:keyboard-dyn-dialogs > mozilla-b2g:master

Looks good to me.
Thanks.

Comment 8

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#qEU8dDPgQdS1gWIsVqNiRQ

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Let's try again...

Comment 10

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#BfkKE-qtSTuF7aJFt-32nA

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Try again with rebase to latest master.

Comment 12

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/d46a9bfb7f7a8f1f903b2e12062a78311793de34