New issue
Advanced search Search tips

Issue 907333 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 902959



Sign in to add a comment

Settings WebUI: Remove neon-animation usage

Project Member Reported by dpa...@chromium.org, Nov 21

Issue description

There is a proposal to bot simplify Settings animation, and remove neon-animation usage in this process. Since there will be multiple CLs, it is better to track this separately. Pasting below from issue 902959.

Status update regarding the Settings removal of neon-animation:

 - CL #1) Remove animations + rewrite main_page_behavior.js. WIP CL at https://chromium-review.googlesource.com/c/chromium/src/+/1324915. Functionality works. Still fixing failing tests.
 - CL #2) Remove neon-animatable https://chromium-review.googlesource.com/c/chromium/src/+/1343657.
- CL #3) Add back some very subtle fade-in transitions (not uploaded yet).



 
Blocking: 902959
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ab0c2e9255d4158565ec08abe9daf72a25c09f6b

commit ab0c2e9255d4158565ec08abe9daf72a25c09f6b
Author: dpapad <dpapad@chromium.org>
Date: Tue Nov 27 00:08:50 2018

Settings WebUI: Fix a11y errors revealed by page/subpage animation changes.

For an unknown reason, the a11y tests currently miss these failures. These are
triggered somehow by the changes in an upcoming CL (1324915), maybe indicating
a timing issue in a11y tests (should be investigated separately,
filed crbug.com/907676).

Tests that are fixed:
 - SettingsAccessibilityTest.ABOUT_image_alt
 - SettingsAccessibilityTest.ABOUT_region
 - SettingsA11ySignOut.SIGN_OUT_button_name

This CL is in preparation of removing neon-animation from Settings WebUI.

Bug:  907333 
Change-Id: I9fd7c03a5f1f6105b144403aa3fc358fcd4333e2
Reviewed-on: https://chromium-review.googlesource.com/c/1345127
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Scott Chen <scottchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610980}
[modify] https://crrev.com/ab0c2e9255d4158565ec08abe9daf72a25c09f6b/chrome/browser/resources/settings/about_page/about_page.html
[modify] https://crrev.com/ab0c2e9255d4158565ec08abe9daf72a25c09f6b/chrome/browser/resources/settings/settings_main/settings_main.html
[modify] https://crrev.com/ab0c2e9255d4158565ec08abe9daf72a25c09f6b/chrome/test/data/webui/settings/a11y/settings_accessibility_test.js

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39

commit 2c1d74f65dea4f02c579b56d8316cfe5ff95dc39
Author: dpapad <dpapad@chromium.org>
Date: Sun Dec 09 21:50:24 2018

Settings WebUI: Remove card and page/subpage animations.

The card expand/collapse animations, as well as the page/subpage animations
are
 - involve lot of JS-driven animation code
 - don't perform great on lower end machines
 - depend on neon-animation elements, which are officially deprecated.

and in general are very hard to maintain/update.

This CL is the first step towards simplifying this part of the code:

 - Remove Web Animations API wrapper (c/b/r/settings/animation/)
 - Remove usage of neon-animated-pages (replaced with iron-pages for now).
 - Re-design/re-factor MainPageBehavior to model a finite state machine,
   see more detailed design at https://goo.gl/npKA3p. With regards to
   Settings URL routes, there are 5 possible states. INITIAL, TOP_LEVEL,
   SECTION, SUBPAGE and DIALOG. There are 19 possible state transitions,
   which are now explicitly handled in the code. Previously it was fairly
   hard to tell which code path corresponds to which case.
 - Change settings-subpage styling, such that when shown it takes up the
   entire viewport height. (previously relied on the parent section to fill
   the viewport).

Bug:  907333 
Change-Id: Iea55ff130a49ef8aa7a111078424d6415c8e03f4
Reviewed-on: https://chromium-review.googlesource.com/c/1324915
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Scott Chen <scottchen@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615011}
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/BUILD.gn
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/about_page/about_page.js
[delete] https://crrev.com/39d4a75a1ef2eb572a779f36c6f1f0a11be86d75/chrome/browser/resources/settings/animation/BUILD.gn
[delete] https://crrev.com/39d4a75a1ef2eb572a779f36c6f1f0a11be86d75/chrome/browser/resources/settings/animation/animation.html
[delete] https://crrev.com/39d4a75a1ef2eb572a779f36c6f1f0a11be86d75/chrome/browser/resources/settings/animation/animation.js
[delete] https://crrev.com/39d4a75a1ef2eb572a779f36c6f1f0a11be86d75/chrome/browser/resources/settings/animation/fade_animations.html
[delete] https://crrev.com/39d4a75a1ef2eb572a779f36c6f1f0a11be86d75/chrome/browser/resources/settings/animation/fade_animations.js
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/basic_page/basic_page.js
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/settings_main/settings_main.js
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/settings_page/BUILD.gn
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/settings_page/main_page_behavior.js
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/settings_page/settings_animated_pages.html
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/settings_page/settings_animated_pages.js
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/settings_page/settings_section.html
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/settings_page/settings_section.js
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/settings_page/settings_subpage.html
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/settings_page/settings_subpage.js
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/settings_page_css.html
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/settings_resources.grd
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/settings_shared_css.html
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/browser/resources/settings/settings_ui/settings_ui.js
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/test/data/webui/BUILD.gn
[delete] https://crrev.com/39d4a75a1ef2eb572a779f36c6f1f0a11be86d75/chrome/test/data/webui/settings/animation_browsertest.js
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/test/data/webui/settings/device_page_tests.js
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/chrome/test/data/webui/settings/settings_main_test.js
[modify] https://crrev.com/2c1d74f65dea4f02c579b56d8316cfe5ff95dc39/testing/buildbot/filters/webui_polymer2_browser_tests.filter

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fea7c851b80582534e5c4318996525bd42e50c30

commit fea7c851b80582534e5c4318996525bd42e50c30
Author: dpapad <dpapad@chromium.org>
Date: Sun Dec 09 22:26:19 2018

Settings WebUI: Fix overscroll behavior corner cases.

After the removal of page/subpage animations, overscroll was not calculated
correctly in some cases. This CL fixes those cases, as well as other cases
that did not work well before at all.

Case 1:
 - Go to chrome://settings/?search=foo
 - From the sidenav  go to chrome://settings/reset. Observe overscroll.

Case 2:
 - Go to chrome://settings/help
 - From the sidenav  go to chrome://settings/reset. Observe overscroll.

Case 3:
 - Go to chrome://settings/printing
 - Scroll a tiny bit so that overscroll padding is reduced by a few pixels.
 - Enter the chrome://settings/cloudPrinters subpage.
 - Exit the subpage via the browser's "back" button. Previous scroll
   position should be restored and overscroll should be the same as before
   entering the subpage.

Bug:  907333 
Change-Id: I99c1f07587524b66cbb9ff0cead2a2ab796ea567
Reviewed-on: https://chromium-review.googlesource.com/c/1361923
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Scott Chen <scottchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615014}
[modify] https://crrev.com/fea7c851b80582534e5c4318996525bd42e50c30/chrome/browser/resources/settings/settings_main/settings_main.html
[modify] https://crrev.com/fea7c851b80582534e5c4318996525bd42e50c30/chrome/browser/resources/settings/settings_main/settings_main.js
[modify] https://crrev.com/fea7c851b80582534e5c4318996525bd42e50c30/chrome/browser/resources/settings/settings_page/main_page_behavior.js
[modify] https://crrev.com/fea7c851b80582534e5c4318996525bd42e50c30/chrome/browser/resources/settings/settings_page/settings_section.js
[modify] https://crrev.com/fea7c851b80582534e5c4318996525bd42e50c30/chrome/test/data/webui/settings/multidevice_page_tests.js
[modify] https://crrev.com/fea7c851b80582534e5c4318996525bd42e50c30/chrome/test/data/webui/settings/settings_main_test.js

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 12

Status: Fixed (was: Started)
neon-animation usage has been removed from Settings, except for neon-animation/web-animations.html which will be addressed separately. This file is just importing to the polyfil at [1] (and is otherwise unrelated to neon-animation).

As a side benefit Settings crisper.js file size is reduced from 350kb to 333kb, ~4.8% reduction.

Also as a result of removing Settings page/subpage animations, 14 bugs (see [2]) have been addressed (fixed or obsolete).


[1] https://cs.chromium.org/chromium/src/third_party/web-animations-js/sources/web-animations-next-lite.min.js
[2] https://bugs.chromium.org/p/chromium/issues/list?can=1&q=id%3A663646%2C664061%2C675557%2C688619%2C709407%2C711153%2C718344%2C734921%2C735829%2C765135%2C822178%2C848689%2C859794%2C875661&colspec=ID+Pri+M+Stars+ReleaseBlock+Component+Status+Owner+Summary+OS+Modified&x=m&y=releaseblock&cells=ids
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f2fb59d296498d8b049bae8dee3ebace00d9e2cb

commit f2fb59d296498d8b049bae8dee3ebace00d9e2cb
Author: dpapad <dpapad@chromium.org>
Date: Thu Dec 13 01:27:13 2018

WebUI cleanup: Remove neon-animation and friends from non-CrOS builds.

In order to do so, also limit iron-dropdown and cr-searchable-drop-down
to ChromeOS builds (not used anywhere else).

Bug:  907333 ,902959
Change-Id: I71be4291aa8ba8e69b25dbba114593b52da08504
Reviewed-on: https://chromium-review.googlesource.com/c/1372054
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Rebekah Potter <rbpotter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616148}
[modify] https://crrev.com/f2fb59d296498d8b049bae8dee3ebace00d9e2cb/chrome/test/data/webui/cr_elements/cr_elements_browsertest.js
[modify] https://crrev.com/f2fb59d296498d8b049bae8dee3ebace00d9e2cb/ui/webui/resources/cr_elements_resources.grdp
[modify] https://crrev.com/f2fb59d296498d8b049bae8dee3ebace00d9e2cb/ui/webui/resources/polymer_resources.grdp

Sign in to add a comment