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

Fix demos’ RTL ↔ LTR toggling in Safari #2348

Merged
merged 2 commits into from
Mar 6, 2018
Merged

Fix demos’ RTL ↔ LTR toggling in Safari #2348

merged 2 commits into from
Mar 6, 2018

Conversation

swashcap
Copy link
Contributor

@swashcap swashcap commented Mar 6, 2018

  • Problem: The right-to-left ↔ left-to-right toggling on the top-app-bar demo is broken in Safari 11.0.3 (on macOS 10.13.3)

  • Solution: Don’t remove the dir attribute on body in the demo: change its value to ltr when the “RTL” checkbox is unchecked

  • Testing:

    1. Get the fix branch set up: 
      git clone [email protected]:swashcap/material-components-web.git mcw2
cd mcw2
npm install
npm run dev
    2. Open http://localhost:8080/top-app-bar.html in Safari
    3. Confirm the “RTL” checkbox works after checked and unchecked

@swashcap
fix(demos): correct RTL ↔ LTR top app bar toggling in Safari
5e1d96a 
This addresses a bug in Safari 11 where removing the `dir=rtl` attribute
on the `body` element does not revert the direction change on many child
elements. Explicitly settint the `dir` back to `ltr` corrects this
behavior.
@swashcap
Copy link
Contributor Author

swashcap commented Mar 6, 2018

Just to elaborate, here’s the current behavior I see:

current-behavior

@swashcap
Copy link
Contributor Author

swashcap commented Mar 6, 2018

Confirmed behavior on iOS 11.2.6. Here’s a reproduction via the iPhone SE simulator:

ios-sim-whoa

@swashcap
Copy link
Contributor Author

swashcap commented Mar 6, 2018 •
edited
Loading

Confirmed this still exists in Safari Tech Preview 50 (for macOS 10.13 build). Wonder if there’s a ticket for a browser bug…

@codecov-io
Copy link

codecov-io commented Mar 6, 2018 •
edited
Loading

Codecov Report

Merging #2348 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2348   +/-   ##
=======================================
  Coverage   98.85%   98.85%           
=======================================
  Files         100      100           
  Lines        4118     4118           
  Branches      527      527           
=======================================
  Hits         4071     4071           
  Misses         47       47

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab85736...32d6283. Read the comment docs.

@swashcap
Copy link
Contributor Author

swashcap commented Mar 6, 2018

Ran through all the demos with RTL checkboxes and found that the card demo also has the s bug. 32d6283 addresses the issue.

Before:

before

After:

after

@swashcap swashcap changed the title fix(demos): correct RTL ↔ LTR top app bar toggling in Safari Fix demos’ RTL ↔ LTR toggling in Safari Mar 6, 2018
@kfranqueiro kfranqueiro self-requested a review March 6, 2018 15:17
@kfranqueiro kfranqueiro self-assigned this Mar 6, 2018
@kfranqueiro
Copy link
Contributor

Thanks so much for the detailed explanation and checking through the rest of the demo pages to find other instances of the problem! This one's definitely a trip; I was able to reproduce it in isolation, too: https://codepen.io/kfranqueiro/pen/mXZNew

@kfranqueiro kfranqueiro merged commit b9000a4 into material-components:master Mar 6, 2018
@swashcap swashcap deleted the bug/top-app-bar-rtl-safari branch March 6, 2018 17:41
acdvorak added a commit that referenced this pull request Mar 8, 2018
@acdvorak
WIP Squashed commit of the following:
5ccfdc1 
commit f17e0d3
Author: Will Ernest <[email protected]>
Date:   Thu Mar 8 13:20:48 2018 -0500

    fix(text-field): Clicking label should focus textfield (#2353)

commit 77b15f4
Author: Bonnie Zhou <[email protected]>
Date:   Wed Mar 7 16:29:57 2018 -0800

    refactor(button): Remove compact variant (#2361)

    BREAKING CHANGE: The compact variant of MDC Button is removed.

commit 35a5cfc
Author: Will Ernest <[email protected]>
Date:   Tue Mar 6 19:54:23 2018 -0500

    fix(toolbar): Fix colors for svg icons. Update custom-toolbar demo (#2331)

    SVG icons in the toolbar will now use the same color as the font icons.

commit dc52201
Author: Andrew C. Dvorak <[email protected]>
Date:   Tue Mar 6 16:00:12 2018 -0800

    fix(theme): Move @alternate annotations for Closure Stylesheets (#2355)

    `@alternate` annotations need to come before the _second_ property. Otherwise, Closure Compiler strips the first property (it does not output it at all).

commit 3c04419
Author: aprigogin <[email protected]>
Date:   Tue Mar 6 14:29:12 2018 -0800

    fix(button): icon in rtl should have margin right flipped. (#2346)

commit b9000a4
Author: Cory Reed <[email protected]>
Date:   Tue Mar 6 07:37:40 2018 -0800

    fix(demos): Correct RTL/LTR toggling in demos in Safari (#2348)

commit ab85736
Author: Andrew C. Dvorak <[email protected]>
Date:   Mon Mar 5 19:00:11 2018 -0500

    fix: Use `var` instead of `const` in menu demo (#2345)

    Safari 9.x and IE 10 do not support `const`

commit dc3d69f
Author: aprigogin <[email protected]>
Date:   Mon Mar 5 15:22:31 2018 -0800

    fix(rtl): Adding noflip annotations to fix downstream rtl issues (#2344)

    * fix(rtl): Adding noflip annotations to fix downstream rtl issues

    * fix(rtl): remove changes to button, will be in separate PR

    * fix(rtl): remove changes to button, will be in separate PR - second attempt.

    * fix(rtl): removed extra whitespace

commit eb4138e
Author: Patrick RoDee <[email protected]>
Date:   Mon Mar 5 14:10:46 2018 -0800

    docs: Update CHANGELOG.md

commit f478610
Author: Patrick RoDee <[email protected]>
Date:   Mon Mar 5 14:10:30 2018 -0800

    chore: Publish

commit 78408bb
Author: Andrew C. Dvorak <[email protected]>
Date:   Mon Mar 5 16:13:02 2018 -0500

    fix: Use `var` instead of `const` in demos/ready.js (#2343)

    Safari 9.x and IE 10 do not support `const`, and `ready.js` is not transpiled to ES5.

commit 49a9449
Author: Will Ernest <[email protected]>
Date:   Mon Mar 5 12:21:54 2018 -0500

    chore(select): Fix JS example in Readme (#2332)

commit bc17291
Author: Will Ernest <[email protected]>
Date:   Fri Mar 2 13:21:07 2018 -0500

    feat(top app bar): Add short top app bar always collapsed feature (#2327)

commit 0ba7d10
Author: Matty Goo <[email protected]>
Date:   Fri Mar 2 11:33:19 2018 -0500

    fix(text-field): disable validation check in setRequired (#2201)

    BREAKING CHANGE: removed setRequired and isRequired from foundation.

commit c14d244
Author: Will Ernest <[email protected]>
Date:   Fri Mar 2 10:37:18 2018 -0500

    chore(select): Fix ID values in demo (#2330)

    Remove unused ID and changed duplicated ID values. Also fixed the parentheses in RTL.

commit ecf4060
Author: Bonnie Zhou <[email protected]>
Date:   Thu Mar 1 16:38:41 2018 -0500

    feat(chips): Change chip color when selected (#2329)

    BREAKING CHANGE: The `mdc-chip--activated` class, `mdc-chip-activated-ink-color` Sass mixin, and the `toggleActive` methods on `MDCChip`/`MDCChipSet` have been renamed to `mdc-chip--selected`, `mdc-chip-selected-ink-color`, and `toggleSelected`, respectively.

commit 4b24b51
Author: Matty Goo <[email protected]>
Date:   Wed Feb 28 15:20:19 2018 -0500

    chore(floating-label): separate label module from text-field (#2237)

    BREAKING CHANGE: must use `.mdc-floating-label` selector instead of `.mdc-text-field__label`

commit fd8d8d9
Author: Will Ernest <[email protected]>
Date:   Wed Feb 28 14:21:55 2018 -0500

    feat(top-app-bar): Implement short top app bar (#2290)

    Adds the short top app bar variant to the top app bar.

commit a9f9bf2
Author: Kenneth G. Franqueiro <[email protected]>
Date:   Wed Feb 28 13:06:41 2018 -0500

    docs(select): Fix front matter for label sub-component (#2323)
@dependabot-preview dependabot-preview bot mentioned this pull request Mar 19, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kfranqueiro kfranqueiro kfranqueiro approved these changes

Assignees

@kfranqueiro kfranqueiro

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@swashcap @codecov-io @kfranqueiro