1019076 - Flash of Cancel button when opening Dialer

Status: RESOLVED FIXED

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

Description

[Anthony Ricaud (:rik)]

This was previously not showing up because the form was using a hidden attribute. But in bug 1013330, we switched to aria-hidden so it is showing up. I think we should switch to using an imported element (because it is not useful during app startup).

Comment 1

[Anthony Ricaud (:rik)]

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

Comment 2

[Doug Sherk (:drs) (inactive)]

I'm not sure that this is really necessary. What do you think of just applying both the |hidden| and |aria-hidden="true"| attrs together? We can unapply the |hidden| attr on a |setTimeout(0)|, or on the first |LazyLoader.load()| call.

Comment 3

[Anthony Ricaud (:rik)]

That would also work but it is still more tasks for Gecko On startup than not loading more DOM.

Comment 4

[Doug Sherk (:drs) (inactive)]

(In reply to Anthony Ricaud (:rik) from comment #3)
> That would also work but it is still more tasks for Gecko On startup than
> not loading more DOM.

I'm not sure I follow since either way requires one task, and I'm sure that lazyloading fragments is way more expensive than queuing up and running a setTimeout(). But there's also the issue code complexity. Loading a fragment for what is effectively 3 lines of HTML seems like overkill to me, especially for something that we never expect to be duplicating. If we use a setTimeout() or LazyLoader.call(), we can comment directly above it and that will tell us in the future exactly what's going on, even if it's a little bit dirty.

Comment 5

[Anthony Ricaud (:rik)]

I want to move into a direction where we "componentize" our code base. This makes unit testing more accurate since it is testing the real HTML that we ship instead of a copy that can be out of date.

Comment 6

[Doug Sherk (:drs) (inactive)]

(In reply to Anthony Ricaud (:rik) from comment #5)
> I want to move into a direction where we "componentize" our code base. This
> makes unit testing more accurate since it is testing the real HTML that we
> ship instead of a copy that can be out of date.

Making major and dubious design and perf decisions to make testing slightly cleaner is a good sign that we're doing something wrong. In this patch, we're actually adding ~3x more code than we're removing from the test files, despite factoring out the actual HTML itself.

Comment 7

[Anthony Ricaud (:rik)]

Clean code is not less lines of code. Also, I said that it makes testing more accurate, not cleaner.

An evolution to have less lines of code would be enabling nested "elements".

Comment 8

[Doug Sherk (:drs) (inactive)]

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

Okay, I still don't really like this, but I don't think it's worth arguing more about. The work is already done too.

I wish we didn't have to set all of the props in the test, but I can't see a way around that.

Comment 9

[Anthony Ricaud (:rik)]

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

I don't like setting all those attributes either. I should look at web components to see if they can help with that.