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

Try Edge toolbar position fix #10059

Closed
wants to merge 2 commits into from
Closed

Try Edge toolbar position fix #10059

wants to merge 2 commits into from

Conversation

jasmussen
Copy link
Contributor

Currently in Edge, the block-docked toolbar is pushed downwards 51px and overlaps content.

This PR adds a hack to fix that. The hack is from https://browserstrangeness.github.io/css_hacks.html, and in my testing runs only in Edge, and doesn't bleed into either Chrome, Firefox, Safari or IE11.

Chrome (unchanged):

screen shot 2018-09-20 at 09 51 13

Edge:

screen shot 2018-09-20 at 09 51 30

Edge scrolled:

screen shot 2018-09-20 at 09 52 05

IE11 (fixed, not sticky):

screen shot 2018-09-20 at 09 52 26

I would appreciate a sanity check on the hack, as well as testing of lots of different post layouts. The reason we combined float with translate in modern browsers is that it allows the toolbar to stay where it needs to in any layout, despite collapsing margins.

This doesn't seem to be an issue in Edge, but worth looking at both different zoom rates, and small viewports in Edge.

Currently in Edge, the block-docked toolbar is pushed downwards 51px and overlaps content.

This PR adds a hack to fix that. The hack is from https://browserstrangeness.github.io/css_hacks.html, and in my testing runs _only_ in Edge, and doesn't bleed into either Chrome, Firefox, Safari or IE11.
@jasmussen jasmussen added the Browser Issues Issues or PRs that are related to browser specific problems label Sep 20, 2018
@jasmussen jasmussen added this to the 4.0 milestone Sep 20, 2018
@jasmussen jasmussen self-assigned this Sep 20, 2018
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well in my testing. I had to add a commit to reset the transform used to move the toolbar upwards in other browsers, otherwise the toolbar was moved up 52px twice by both the margin-top and transform rules.

@brandonpayton
Copy link
Member

Hi @jasmussen, I'm afraid I might be missing something.

Currently in Edge, the block-docked toolbar is pushed downwards 51px and overlaps content.

Edge:
screen shot 2018-09-20 at 09 51 30

Is this the "block-docked toolbar" in Edge? Can you help me see what is wrong? Everything looks fine to me, and in testing, content overlap only happens when scrolling so the toolbar sticks to the header.

@jasmussen
Copy link
Contributor Author

Thanks for the reviews!

@brandonpayton sorry I should've been more clear, been going a little fast on my PRs this past week. What you're looking at is the after image.

Here are more expansive before and after images. Before:

before

Here's after this branch is applied:

after

Copy link
Member

@brandonpayton brandonpayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like something odd is going on here. I'm not able to reproduce this issue on master, and even though the comment on the margin-top says it's necessary because translate doesn't work, we had to specify zero translation so we wouldn't have excessive toolbar offset.

Here's what I'm seeing in Microsoft Edge 42.17134.1.0.

Toolbar position with master:
sticky-toolbar-on-edge

Toolbar position with initial fix:
sticky-toolbar-on-edge-initial-fix

Toolbar position with fix + specifying zero translation:
sticky-toolbar-on-edge-initial-fix-zero-translate

I wonder if there is some difference in our environments causing this inconsistency.

margin-top: -$block-toolbar-height -$block-padding -$border-width;

// Disable the technique used to move the block toolbar upwards in non-edge browsers.
transform: translateY(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The need for this is puzzling because the comment on the margin-top above indicates that translate isn't working.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is very puzzling indeed. Perhaps you are on a newer version of Edge than my VM, and perhaps that version of Edge fixes this. I'm working from my home office today so it's a bit harder for me to test, so I would love additional eyes on how this toolbar works across versions of Edge, because right now I can't reproduce.

Copy link
Contributor

@talldan talldan Sep 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is odd. I'm also now seeing that it looks fine in master. Maybe my cache hadn't cleared when I switched to master to test it.

I'm also on 42.17134.1.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm. That's interesting indeed. I'm adding "In progress" label for now, and removing milestone, just so we don't end up fixing something that's fixed in the browser itself. I will be sure to check the browser version in my coworking space tomorrow, see if it's an older one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iiiiiinteresting, the plot thickens. Behold my Edge version:

screen shot 2018-09-25 at 10 07 58

And still seeing the issue:

screen shot 2018-09-25 at 10 07 43

Stay tuned as I download a new vbox image with a newer version and try again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, it's fixed in the newer version:

screen shot 2018-09-25 at 12 02 32

screen shot 2018-09-25 at 12 02 41

Since Edge auto updates, I'm ging to close this one as now obsolete. Wohoo! Thanks for the sanity checks.

@jasmussen jasmussen added the [Status] In Progress Tracking issues with work in progress label Sep 24, 2018
@jasmussen jasmussen removed this from the 4.0 milestone Sep 24, 2018
@jasmussen jasmussen closed this Sep 25, 2018
@jasmussen jasmussen deleted the try/fix-edge-sticky branch September 25, 2018 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Browser Issues Issues or PRs that are related to browser specific problems [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants