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 demos’ RTL ↔ LTR toggling in Safari #2348

Merged

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 git@github.com: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

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

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

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
commit f17e0d3
Author: Will Ernest <34519388+williamernest@users.noreply.github.com>
Date:   Thu Mar 8 13:20:48 2018 -0500

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

commit 77b15f4
Author: Bonnie Zhou <bzbee003@gmail.com>
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 <34519388+williamernest@users.noreply.github.com>
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 <acdvorak@gmail.com>
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 <17075403+aprigogin@users.noreply.github.com>
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 <swashcap@gmail.com>
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 <acdvorak@gmail.com>
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 <17075403+aprigogin@users.noreply.github.com>
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 <prodee@google.com>
Date:   Mon Mar 5 14:10:46 2018 -0800

    docs: Update CHANGELOG.md

commit f478610
Author: Patrick RoDee <prodee@google.com>
Date:   Mon Mar 5 14:10:30 2018 -0800

    chore: Publish

commit 78408bb
Author: Andrew C. Dvorak <acdvorak@gmail.com>
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 <34519388+williamernest@users.noreply.github.com>
Date:   Mon Mar 5 12:21:54 2018 -0500

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

commit bc17291
Author: Will Ernest <34519388+williamernest@users.noreply.github.com>
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 <mattgoo@google.com>
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 <34519388+williamernest@users.noreply.github.com>
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 <bzbee003@gmail.com>
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 <mattgoo@google.com>
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 <34519388+williamernest@users.noreply.github.com>
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 <kfranqueiro@users.noreply.github.com>
Date:   Wed Feb 28 13:06:41 2018 -0500

    docs(select): Fix front matter for label sub-component (#2323)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants