Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] web, website: adapt menu loading to jQuery 3 #32465

Closed

Conversation

Projects
None yet
3 participants
@qsm-odoo
Copy link
Contributor

commented Apr 5, 2019

Javascript is responsible of showing the menu, potentially with some
parts regrouped in an extra dropdown, on page load (this is what we
call the "autohide" feature). This was done on both the menu and the
affix menu. It worked on the affix menu because the affix menu widget
was initialized before (by luck: alphabetical order).

Since the jQuery 3 update bb004e3
deferred are always delayed (even if their resolve condition is met on
creation). This made the animations to be initialized in the same order
as before but their 'start' method to be delayed (as the willStart was
delayed). So what happened before was:

  1. Find menus to enable affix
  2. Enable affix on found menus (duplicate the menus to do that)
  3. Find menus to enable autohide
  4. Enable autohide on found menus (both the menu and the duplicated one
    were found)

And since jQuery 3:

  1. Find menus to enable affix
  2. Find menus to enable autohide
  3. Enable affix on found menus (duplicate the menus to do that)
  4. Enable autohide on found menus (the duplicated one was not found at
    step 2 since it did not exist yet)

In fact, widget starting should never have relied on alphabetical order.
The fix only consists of starting widgets on the duplicated menu once it
is ready. This is what this commit does.

However, doing this revealed another bug: if the widgets are started
only for a DOM portion like a '.a' element, a widget with a multiple
parts selector like a '.a .b' selector will not be considered, as
searching inside a '.a' element will not allow to match elements with
a selector containing a '.a'. This is in fact not really a "bug" but a
limitation, so unless we get problems because of this in stable
versions, we can only "fix" this for the current saas. This is fixed
thanks to dom.cssFind which already existed. This commit however
improves it to try making it more efficient.

@qsm-odoo qsm-odoo self-assigned this Apr 5, 2019

@robodoo robodoo added the seen 🙂 label Apr 5, 2019

@qsm-odoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

@dja-odoo This is the fix for the menu items not showing in the affix menu bar. I will merge it once I tested some things a little bit more.

@dja-odoo

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

@qsm-odoo Great ! Thanks bud !

@qsm-odoo qsm-odoo force-pushed the odoo-dev:saas-12.3-fix-autohide-starting-qsm branch from 00820a5 to 2492188 Apr 8, 2019

@qsm-odoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

@robodoo robodoo added the r+ 👌 label Apr 8, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

'ci/runbot' failed on this reviewed PR.

1 similar comment
@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

'ci/runbot' failed on this reviewed PR.

@qsm-odoo qsm-odoo force-pushed the odoo-dev:saas-12.3-fix-autohide-starting-qsm branch from 2492188 to d156b0b Apr 16, 2019

@robodoo robodoo removed the r+ 👌 label Apr 16, 2019

@qsm-odoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

@robodoo robodoo added the r+ 👌 label Apr 16, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

'ci/runbot' failed on this reviewed PR.

1 similar comment
@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

'ci/runbot' failed on this reviewed PR.

@qsm-odoo qsm-odoo force-pushed the odoo-dev:saas-12.3-fix-autohide-starting-qsm branch from d156b0b to 6d370fd Apr 17, 2019

@robodoo robodoo removed the r+ 👌 label Apr 17, 2019

@qsm-odoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2019

@robodoo robodoo added the r+ 👌 label Apr 17, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

'ci/runbot' failed on this reviewed PR.

2 similar comments
@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

'ci/runbot' failed on this reviewed PR.

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

'ci/runbot' failed on this reviewed PR.

@qsm-odoo qsm-odoo force-pushed the odoo-dev:saas-12.3-fix-autohide-starting-qsm branch from 6d370fd to 76f6f7c Apr 19, 2019

@robodoo robodoo removed the r+ 👌 label Apr 19, 2019

@qsm-odoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

@robodoo robodoo added the r+ 👌 label Apr 19, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

'ci/runbot' failed on this reviewed PR.

[FIX] web, website: adapt menu loading to jQuery 3
Javascript is responsible of showing the menu, potentially with some
parts regrouped in an extra dropdown, on page load (this is what we
call the "autohide" feature). This was done on both the menu and the
affix menu. It worked on the affix menu because the affix menu widget
was initialized before (by luck: alphabetical order).

Since the jQuery 3 update bb004e3
deferred are always delayed (even if their resolve condition is met on
creation). This made the animations to be initialized in the same order
as before but their 'start' method to be delayed (as the willStart was
delayed). So what happened before was:

1) Find menus to enable affix
2) Enable affix on found menus (duplicate the menus to do that)
3) Find menus to enable autohide
4) Enable autohide on found menus (both the menu and the duplicated one
   were found)

And since jQuery 3:

1) Find menus to enable affix
2) Find menus to enable autohide
3) Enable affix on found menus (duplicate the menus to do that)
4) Enable autohide on found menus (the duplicated one was not found at
   step 2 since it did not exist yet)

In fact, widget starting should never have relied on alphabetical order.
The fix only consists of starting widgets on the duplicated menu once it
is ready. This is what this commit does.

However, doing this revealed another bug: if the widgets are started
only for a DOM portion like a '.a' element, a widget with a multiple
parts selector like a '.a .b' selector will not be considered, as
searching inside a '.a' element will not allow to match elements with
a selector containing a '.a'. This is in fact not really a "bug" but a
limitation, so unless we get problems because of this in stable
versions, we can only "fix" this for the current saas. This is fixed
thanks to `dom.cssFind` which already existed. This commit however
improves it to try making it more efficient.

@qsm-odoo qsm-odoo force-pushed the odoo-dev:saas-12.3-fix-autohide-starting-qsm branch from 76f6f7c to dacb15d Apr 19, 2019

@robodoo robodoo removed the r+ 👌 label Apr 19, 2019

@qsm-odoo

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

robodoo pushed a commit that referenced this pull request Apr 19, 2019

[FIX] web, website: adapt menu loading to jQuery 3
Javascript is responsible of showing the menu, potentially with some
parts regrouped in an extra dropdown, on page load (this is what we
call the "autohide" feature). This was done on both the menu and the
affix menu. It worked on the affix menu because the affix menu widget
was initialized before (by luck: alphabetical order).

Since the jQuery 3 update bb004e3
deferred are always delayed (even if their resolve condition is met on
creation). This made the animations to be initialized in the same order
as before but their 'start' method to be delayed (as the willStart was
delayed). So what happened before was:

1) Find menus to enable affix
2) Enable affix on found menus (duplicate the menus to do that)
3) Find menus to enable autohide
4) Enable autohide on found menus (both the menu and the duplicated one
   were found)

And since jQuery 3:

1) Find menus to enable affix
2) Find menus to enable autohide
3) Enable affix on found menus (duplicate the menus to do that)
4) Enable autohide on found menus (the duplicated one was not found at
   step 2 since it did not exist yet)

In fact, widget starting should never have relied on alphabetical order.
The fix only consists of starting widgets on the duplicated menu once it
is ready. This is what this commit does.

However, doing this revealed another bug: if the widgets are started
only for a DOM portion like a '.a' element, a widget with a multiple
parts selector like a '.a .b' selector will not be considered, as
searching inside a '.a' element will not allow to match elements with
a selector containing a '.a'. This is in fact not really a "bug" but a
limitation, so unless we get problems because of this in stable
versions, we can only "fix" this for the current saas. This is fixed
thanks to `dom.cssFind` which already existed. This commit however
improves it to try making it more efficient.

closes #32465

Signed-off-by: Quentin Smetz (qsm) <qsm@odoo.com>
@robodoo

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

Merged, thanks!

@robodoo robodoo closed this Apr 19, 2019

@qsm-odoo qsm-odoo deleted the odoo-dev:saas-12.3-fix-autohide-starting-qsm branch Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.