Use hidden attribute rather the 'display: none' for hiding menus.

Use hidden attribute rather the 'display: none' for hiding menus.

BUG=None
TEST=Manual test of menus on NTP, bookmarks, chrome://history.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138230

[Patrick Dubroy, 2012-05-08 11:06:59]

PTAL. Turns out this was pretty simple.

[Evan Stade, 2012-05-08 18:50:29]

lgtm

did you test this on all the pages that uses menu.js? I wouldn't expect problems
but you never know till you try.

[arv (Not doing code reviews), 2012-05-08 21:46:25]

LGTM

http://codereview.chromium.org/10389016/diff/1/chrome/browser/resources/share...
File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right):

http://codereview.chromium.org/10389016/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/js/cr/ui/menu_button.js:138:
this.removeAttribute('menu-shown');
I think we can remove this attribute now since hidden is an attribute that we
can select on.

[Patrick Dubroy, 2012-05-09 14:39:35]

Yep, tested all the places that use menu.js, including on ChromeOS.

http://codereview.chromium.org/10389016/diff/1/chrome/browser/resources/share...
File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right):

http://codereview.chromium.org/10389016/diff/1/chrome/browser/resources/share...
chrome/browser/resources/shared/js/cr/ui/menu_button.js:138:
this.removeAttribute('menu-shown');
On 2012/05/08 21:46:25, arv wrote:
> I think we can remove this attribute now since hidden is an attribute that we
> can select on.

I think we still need it for styling. There are a bunch of CSS rules that use
it, and I don't think there's an easy way to change them so that they don't.

[Patrick Dubroy, 2012-05-09 15:29:37]

I just realized that there is a problem with setting 'hidden = true' in the
constructor: a menu element that is not hidden by default will briefly flash on
screen. The old code didn't have this problem because the default menu style was
'display: none'.

What do you guys think we should do? Just remove that line, and force anyone who
uses a menu to explicitly mark it as hidden initially?

On 2012/05/09 14:39:35, dubroy wrote:
> Yep, tested all the places that use menu.js, including on ChromeOS.
> 
>
http://codereview.chromium.org/10389016/diff/1/chrome/browser/resources/share...
> File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right):
> 
>
http://codereview.chromium.org/10389016/diff/1/chrome/browser/resources/share...
> chrome/browser/resources/shared/js/cr/ui/menu_button.js:138:
> this.removeAttribute('menu-shown');
> On 2012/05/08 21:46:25, arv wrote:
> > I think we can remove this attribute now since hidden is an attribute that
we
> > can select on.
> 
> I think we still need it for styling. There are a bunch of CSS rules that use
> it, and I don't think there's an easy way to change them so that they don't.

[arv (Not doing code reviews), 2012-05-09 17:29:53]

On 2012/05/09 15:29:37, dubroy wrote:
> I just realized that there is a problem with setting 'hidden = true' in the
> constructor: a menu element that is not hidden by default will briefly flash
on
> screen. The old code didn't have this problem because the default menu style
was
> 'display: none'.
> 
> What do you guys think we should do? Just remove that line, and force anyone
who
> uses a menu to explicitly mark it as hidden initially?

This rings a bell. That was why I had display: none in the first place :'(

Unfortunately, due to https://bugs.webkit.org/show_bug.cgi?id=49739 we cannot
override the hidden accessor cleanly. We could do it per instance but it is not
pretty.

Inside decorate we can define a getter/setter on the instance that also updates
the style.display as needed.

this.hidden = true;
Object.defineOwnProperty(this, 'hidden', {
  get: function() {
    return this.hasAttribute('hidden');
  },
  set: function(v) {
    this.style.display = v ? 'block' : '';
    this.setAttribute('hidden', '');
  },
  enumerable: true,
  configurable: true
});

[arv (Not doing code reviews), 2012-05-09 17:31:53]

On 2012/05/09 14:39:35, dubroy wrote:
> Yep, tested all the places that use menu.js, including on ChromeOS.
> 
>
http://codereview.chromium.org/10389016/diff/1/chrome/browser/resources/share...
> File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right):
> 
>
http://codereview.chromium.org/10389016/diff/1/chrome/browser/resources/share...
> chrome/browser/resources/shared/js/cr/ui/menu_button.js:138:
> this.removeAttribute('menu-shown');
> On 2012/05/08 21:46:25, arv wrote:
> > I think we can remove this attribute now since hidden is an attribute that
we
> > can select on.
> 
> I think we still need it for styling. There are a bunch of CSS rules that use
> it, and I don't think there's an easy way to change them so that they don't.

My point was that we could use [hidden] and :not([hidden]) in the selectors
instead.

[Patrick Dubroy, 2012-05-09 17:40:47]

On 2012/05/09 17:31:53, arv wrote:
> On 2012/05/09 14:39:35, dubroy wrote:
> > Yep, tested all the places that use menu.js, including on ChromeOS.
> > 
> >
>
http://codereview.chromium.org/10389016/diff/1/chrome/browser/resources/share...
> > File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right):
> > 
> >
>
http://codereview.chromium.org/10389016/diff/1/chrome/browser/resources/share...
> > chrome/browser/resources/shared/js/cr/ui/menu_button.js:138:
> > this.removeAttribute('menu-shown');
> > On 2012/05/08 21:46:25, arv wrote:
> > > I think we can remove this attribute now since hidden is an attribute that
> we
> > > can select on.
> > 
> > I think we still need it for styling. There are a bunch of CSS rules that
use
> > it, and I don't think there's an easy way to change them so that they don't.
> 
> My point was that we could use [hidden] and :not([hidden]) in the selectors
> instead.

Note that that class is on the menu _button_, not the menu itself. Unless we
force a specific relationship between the button and the menu (e.g. parent/child
or sibling), we can't use the hidden attribute on the menu to style the button.

[arv (Not doing code reviews), 2012-05-09 17:56:56]

On 2012/05/09 17:40:47, dubroy wrote:
> On 2012/05/09 17:31:53, arv wrote:
> > On 2012/05/09 14:39:35, dubroy wrote:
> > > Yep, tested all the places that use menu.js, including on ChromeOS.
> > > 
> > >
> >
>
http://codereview.chromium.org/10389016/diff/1/chrome/browser/resources/share...
> > > File chrome/browser/resources/shared/js/cr/ui/menu_button.js (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10389016/diff/1/chrome/browser/resources/share...
> > > chrome/browser/resources/shared/js/cr/ui/menu_button.js:138:
> > > this.removeAttribute('menu-shown');
> > > On 2012/05/08 21:46:25, arv wrote:
> > > > I think we can remove this attribute now since hidden is an attribute
that
> > we
> > > > can select on.
> > > 
> > > I think we still need it for styling. There are a bunch of CSS rules that
> use
> > > it, and I don't think there's an easy way to change them so that they
don't.
> > 
> > My point was that we could use [hidden] and :not([hidden]) in the selectors
> > instead.
> 
> Note that that class is on the menu _button_, not the menu itself. Unless we
> force a specific relationship between the button and the menu (e.g.
parent/child
> or sibling), we can't use the hidden attribute on the menu to style the
button.

Never mind me, nothing to see here...

[Evan Stade, 2012-05-10 21:38:28]

well you could do this:

  menu:not(.decorated) {
    display: none;
  }

then in decorate,

  this.classList.add('decorated');

[Patrick Dubroy, 2012-05-21 09:38:25]

Resurrecting this CL, and fixed the FoUC using Evan's suggestion.

On 2012/05/10 21:38:28, Evan Stade wrote:
> well you could do this:
> 
>   menu:not(.decorated) {
>     display: none;
>   }
> 
> then in decorate,
> 
>   this.classList.add('decorated');

[Evan Stade, 2012-05-21 20:23:55]

lgtm