Hide accessibility options for unimplemented features.

Hide accessibility options for unimplemented features.


R=dmazzoni@chromium.org
BUG=chromium-os:25272
TEST=Open the systems options page, make sure there is only one a11y option


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120040

[Zachary Kuznia, 2012-01-27 06:17:55]

On 2012/01/27 06:12:07, Zachary Kuznia wrote:

Hi James,

Could you do an OWNERS review on this?

Thanks,
-Zach

[dmazzoni, 2012-01-27 06:48:19]

lgtm with display:none instead of visibility:hidden. thanks.

https://chromiumcodereview.appspot.com/9295010/diff/1/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/system_options.html (right):

https://chromiumcodereview.appspot.com/9295010/diff/1/chrome/browser/resource...
chrome/browser/resources/options/chromeos/system_options.html:125: <div
class="option-name" style="visibility: hidden">
I think display: none would be best. visibility: hidden can still affect layout.

[Zachary Kuznia, 2012-01-27 07:56:36]

https://chromiumcodereview.appspot.com/9295010/diff/1/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/system_options.html (right):

https://chromiumcodereview.appspot.com/9295010/diff/1/chrome/browser/resource...
chrome/browser/resources/options/chromeos/system_options.html:125: <div
class="option-name" style="visibility: hidden">
On 2012/01/27 06:48:19, Dominic Mazzoni wrote:
> I think display: none would be best. visibility: hidden can still affect
layout.

Done.

[Zachary Kuznia, 2012-01-30 06:09:32]

jhawkins: Ping

[James Hawkins, 2012-01-30 17:32:39]

Though I'd rather not have these options in the HTML at all until they're
implemented.

https://chromiumcodereview.appspot.com/9295010/diff/4001/chrome/browser/resou...
File chrome/browser/resources/options/chromeos/system_options.html (right):

https://chromiumcodereview.appspot.com/9295010/diff/4001/chrome/browser/resou...
chrome/browser/resources/options/chromeos/system_options.html:125: <div
class="option-name" style="display: none">
Use hidden attribute instead.

[Zachary Kuznia, 2012-01-31 02:34:07]

https://chromiumcodereview.appspot.com/9295010/diff/4001/chrome/browser/resou...
File chrome/browser/resources/options/chromeos/system_options.html (right):

https://chromiumcodereview.appspot.com/9295010/diff/4001/chrome/browser/resou...
chrome/browser/resources/options/chromeos/system_options.html:125: <div
class="option-name" style="display: none">
On 2012/01/30 17:32:39, James Hawkins wrote:
> Use hidden attribute instead.

Done.

[James Hawkins, 2012-01-31 18:49:07]

I'd rather remove UI for unimplemented features and add it back as they are
implemented (due to it being dead code).

https://chromiumcodereview.appspot.com/9295010/diff/8001/chrome/browser/resou...
File chrome/browser/resources/options/chromeos/system_options.html (right):

https://chromiumcodereview.appspot.com/9295010/diff/8001/chrome/browser/resou...
chrome/browser/resources/options/chromeos/system_options.html:125: <div
class="option-name" hidden="true">
For future reference, hidden is an empty attribute so you shouldn't set it to a
'true' value.

[dmazzoni, 2012-01-31 19:47:15]

I'm not sure I agree with the objection to dead code when it's actively being
worked on; the codebase is full of code that is under development - either with
no UI yet, or hidden behind a flag. This doesn't seem different to me.

One reason we wanted the various options to be part of the HTML sooner rather
than later was so that ongoing UI design work would take into account that
accessibility is a subcategory with several options, not a single boolean.

It's not a big deal either way, though. The code to flip the various
accessibility switches is still there so we can finish development and testing,
and we can easily add the html back later.

[James Hawkins, 2012-01-31 20:01:14]

On 2012/01/31 19:47:15, Dominic Mazzoni wrote:
> I'm not sure I agree with the objection to dead code when it's actively being
> worked on; the codebase is full of code that is under development - either
with
> no UI yet, or hidden behind a flag. This doesn't seem different to me.
> 
> One reason we wanted the various options to be part of the HTML sooner rather
> than later was so that ongoing UI design work would take into account that
> accessibility is a subcategory with several options, not a single boolean.
> 
> It's not a big deal either way, though. The code to flip the various
> accessibility switches is still there so we can finish development and
testing,
> and we can easily add the html back later.

The fact that it's trivial to add this UI back is why I'm insisting it be
removed until it's functional.

[Zachary Kuznia, 2012-01-31 20:17:39]

On 2012/01/31 20:01:14, James Hawkins wrote:
> On 2012/01/31 19:47:15, Dominic Mazzoni wrote:
> > I'm not sure I agree with the objection to dead code when it's actively
being
> > worked on; the codebase is full of code that is under development - either
> with
> > no UI yet, or hidden behind a flag. This doesn't seem different to me.
> > 
> > One reason we wanted the various options to be part of the HTML sooner
rather
> > than later was so that ongoing UI design work would take into account that
> > accessibility is a subcategory with several options, not a single boolean.
> > 
> > It's not a big deal either way, though. The code to flip the various
> > accessibility switches is still there so we can finish development and
> testing,
> > and we can easily add the html back later.
> 
> The fact that it's trivial to add this UI back is why I'm insisting it be
> removed until it's functional.

Removed the UI for the options.

[James Hawkins, 2012-01-31 20:19:11]

LGTM, thanks

[commit-bot: I haz the power, 2012-02-01 04:17:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/9295010/14002

[commit-bot: I haz the power, 2012-02-01 05:46:36]

Change committed as 120040