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.

Sign up for GitHub

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

Jump to bottom

bug(Breakpoints): Incorrect upper boundary lead to wrong match result #20850

Closed
tomrlq opened this issue Oct 22, 2020 · 3 comments · Fixed by #20866
Closed

bug(Breakpoints): Incorrect upper boundary lead to wrong match result #20850

tomrlq opened this issue Oct 22, 2020 · 3 comments · Fixed by #20866
Labels
area: cdk/layout help wanted The team would appreciate a PR from the community to address this issue P4 A relatively minor issue that is not relevant to core functions

Comments

@tomrlq
Copy link
Contributor

tomrlq commented Oct 22, 2020 •
edited

Reproduction

Use StackBlitz to reproduce your issue: https://components-issue-r4nh9s.stackblitz.io

Steps to reproduce:

  1. change browser size to 960 * 600
  2. apply following code
this.breakpointObserver.observe(Breakpoints.Handset).subscribe((result) => {
    this.isHandset = result.matches;
});

Expected Behavior

matching result should be false

Actual Behavior

matching result is true

Environment

  • Angular: 5, 6, 7, 8, 9, 10
  • CDK/Material: 5, 6, 7, 8, 9, 10
  • Browser(s): Chrome, Firefox, Safari, Edge
  • Operating System: Windows, macOS, Ubuntu

How to fix

If we take a look at Bootstrap's source code, we can see that, their upper boundary is using numbers like xxx.98, while in Angular Material, the upper boundary is using numbers like xxx.99, and this lead to the bug.
I take size 960*600 as example, in browser, if you match something like

'(max-width: 599.99px) and (orientation: portrait), ' +
 '(max-width: 959.99px) and (orientation: landscape)'

browser will give you true, only if you match something like

'(max-width: 599.98px) and (orientation: portrait), ' +
 '(max-width: 959.98px) and (orientation: landscape)'

browser will give you false
@tomrlq tomrlq added the needs triage This issue needs to be triaged by the team label Oct 22, 2020
@tomrlq tomrlq changed the title bug(Breakpoints): Incorrect upper boundary number lead to wrong match result bug(Breakpoints): Incorrect upper boundary lead to wrong match result Oct 22, 2020
@jelbourn jelbourn added area: cdk/layout help wanted The team would appreciate a PR from the community to address this issue P4 A relatively minor issue that is not relevant to core functions and removed needs triage This issue needs to be triaged by the team labels Oct 22, 2020
@jelbourn
Copy link
Member

Confirmed that .98 falls under the boundary and .99 counts as over. Feel free to send a PR!

tomrlq added a commit to tomrlq/components that referenced this issue Oct 23, 2020
@tomrlq
Fix for angular#20850
dfc2834
@tomrlq
Copy link
Contributor Author

tomrlq commented Oct 23, 2020 •
edited

#20866
I have just submitted a PR, thanks

tomrlq added a commit to tomrlq/components that referenced this issue Oct 26, 2020
@tomrlq
fix(cdk/layout): decrease breakpoint upper bounds
f1ac9b3 
The previous breakpoint upper bounds use `.99` for the
fractional components of upper bounds. However, this seems
to cause browser to round up to the next pixel, creating an overlap
for these values. Changing to `.98` fixes this.

Fixes angular#20850
@tomrlq tomrlq mentioned this issue Oct 26, 2020
Merged
annieyw pushed a commit that referenced this issue Nov 3, 2020
@tomrlq
fix(cdk/layout): decrease breakpoint upper bounds (#20866)
16576e4 
The previous breakpoint upper bounds use `.99` for the
fractional components of upper bounds. However, this seems
to cause browser to round up to the next pixel, creating an overlap
for these values. Changing to `.98` fixes this.

Fixes #20850
annieyw pushed a commit that referenced this issue Nov 3, 2020
@tomrlq @annieyw
fix(cdk/layout): decrease breakpoint upper bounds (#20866)
cf7267e 
The previous breakpoint upper bounds use `.99` for the
fractional components of upper bounds. However, this seems
to cause browser to round up to the next pixel, creating an overlap
for these values. Changing to `.98` fixes this.

Fixes #20850

(cherry picked from commit 16576e4)
annieyw pushed a commit that referenced this issue Nov 3, 2020
@tomrlq @annieyw
fix(cdk/layout): decrease breakpoint upper bounds (#20866)
0256866 
The previous breakpoint upper bounds use `.99` for the
fractional components of upper bounds. However, this seems
to cause browser to round up to the next pixel, creating an overlap
for these values. Changing to `.98` fixes this.

Fixes #20850

(cherry picked from commit 16576e4)
@CaerusKaru CaerusKaru mentioned this issue Nov 12, 2020
Merged
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 4, 2020
wagnermaciel pushed a commit to wagnermaciel/components that referenced this issue Jan 14, 2021
@tomrlq @wagnermaciel
fix(cdk/layout): decrease breakpoint upper bounds (angular#20866)
eb65158 
The previous breakpoint upper bounds use `.99` for the
fractional components of upper bounds. However, this seems
to cause browser to round up to the next pixel, creating an overlap
for these values. Changing to `.98` fixes this.

Fixes angular#20850
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area: cdk/layout help wanted The team would appreciate a PR from the community to address this issue P4 A relatively minor issue that is not relevant to core functions
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(cdk/layout): decrease breakpoint upper bounds tomrlq/components
2 participants
@jelbourn @tomrlq