Print Preview: Fixing tab order of hidden elements.

Print Preview: Fixing tab order of hidden elements.

BUG=103997
TEST=Changing focus using tab should only navigate to visible elements.


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

[dpapad, 2011-11-15 21:34:27]

Fixed the event handlers that execute when an animation ends (they were only
executing for fade in animations). After that fixing tab order was trivial.

[Evan Stade, 2011-11-16 04:16:31]

http://codereview.chromium.org/8573016/diff/6/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/print_preview_animations.js (right):

http://codereview.chromium.org/8573016/diff/6/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview_animations.js:80:
eventTracker.add(el, 'webkitTransitionEnd',
shuoldn't this be Animation not Transition

[dpapad, 2011-11-16 19:37:28]

http://codereview.chromium.org/8573016/diff/6/chrome/browser/resources/print_...
File chrome/browser/resources/print_preview/print_preview_animations.js (right):

http://codereview.chromium.org/8573016/diff/6/chrome/browser/resources/print_...
chrome/browser/resources/print_preview/print_preview_animations.js:80:
eventTracker.add(el, 'webkitTransitionEnd',
On 2011/11/16 04:16:31, Evan Stade wrote:
> shuoldn't this be Animation not Transition

No, it is transition on purpose, since the fading out is completed by a
transition.

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview_animations.js (right):

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_animations.js:67: var
animName = addAnimation('');
Removed some unneeded animation code for the fade out.

[Evan Stade, 2011-11-16 20:04:23]

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview.css (left):

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.css:169:
-webkit-transition: color 200ms;
why remove?

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview_animations.js (right):

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_animations.js:71: false );
false);

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_animations.js:79: *
@param{EventTracker} The |EventTracker| object that was used for adding this
param space

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_animations.js:83: function
onFadeOutAnimationEnd(animationName, eventTracker, event) {
call it onFadeOutTransitionEnd

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_animations.js:83: function
onFadeOutAnimationEnd(animationName, eventTracker, event) {
you should make sure the property that is done transitioning is the one you
expected to be done transitioning

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_animations.js:85:
eventTracker.remove(event.target, 'webkitTransitionEnd');
use this instead of event.target

[dpapad, 2011-11-16 21:09:25]

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview.css (left):

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.css:169:
-webkit-transition: color 200ms;
On 2011/11/16 20:04:23, Evan Stade wrote:
> why remove?

This only makes sense when there is some text, which happens in the case of
animating hints already (line 136), so basically it had no effect here.

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview_animations.js (right):

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_animations.js:71: false );
On 2011/11/16 20:04:23, Evan Stade wrote:
> false);

Done.

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_animations.js:79: *
@param{EventTracker} The |EventTracker| object that was used for adding this
On 2011/11/16 20:04:23, Evan Stade wrote:
> param space

Done.

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_animations.js:83: function
onFadeOutAnimationEnd(animationName, eventTracker, event) {
On 2011/11/16 20:04:23, Evan Stade wrote:
> you should make sure the property that is done transitioning is the one you
> expected to be done transitioning

Done.

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_animations.js:83: function
onFadeOutAnimationEnd(animationName, eventTracker, event) {
On 2011/11/16 20:04:23, Evan Stade wrote:
> call it onFadeOutTransitionEnd

Done.

http://codereview.chromium.org/8573016/diff/5001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview_animations.js:85:
eventTracker.remove(event.target, 'webkitTransitionEnd');
On 2011/11/16 20:04:23, Evan Stade wrote:
> use this instead of event.target

Done.

[Evan Stade, 2011-11-17 00:34:19]

lgtm

[commit-bot: I haz the power, 2011-11-17 02:05:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpapad@chromium.org/8573016/9002

[commit-bot: I haz the power, 2011-11-17 06:48:34]

Change committed as 110461