h5bp / main.css

A repository for the development of the HTML5 Boilerplate CSS file, main.css
MIT License
206 stars 44 forks source link

macOS - VoiceOver / Chrome announcing visually hidden text out of order #12

Open joe-watkins opened 7 years ago

joe-watkins commented 7 years ago

I'm submitting a ...

Steps to reproduce

  1. Visit https://codepen.io/joe-watkins/pen/OjpqxL with Safari or Chrome and VoiceOver activated.
  2. Tab to each link and listen to how they are announced.

Expected Results

The visually hidden text using HTML5 boilerplate's .visuallyhidden {} will be announced in the order that it is within the markup.

Actual Results

The order of the text is not announced does not match the markup order. Visually hidden text is announced before visible text copy.

Potential fix

After some testing, I've found that using position: relative; and display: inline-block; appears to solve this. e.g.

.visuallyhidden {
    border: 0;
    clip: rect(0 0 0 0);
    clip-path: inset(50%);
    width: 1px;
    height: 1px;
    margin: -1px;
    overflow: hidden;
    padding: 0;
    position: relative; // different - for reading order in macOS VO
    display: inline-block; // new - for reading order in macOS VO
}

SR Tested (works in)

Browsers tested (works - hides text in)

Video screencast of experience

Everything Is AWESOME

I'd love to get some thoughts on this and I'd be happy to create a PR if everyone is into it :)

scottaohara commented 7 years ago

Doing some additional testing on this and I've identified that it's the negative margin that is specifically to blame for the incorrect reading order.

If you either delete the negative margin or set the margin to zero, the correct reading order is reinstated with no additional change to the rule set needed.

Awesome find @joe-watkins

roblarsen commented 7 years ago

🥇 bug report @joe-watkins

Thanks for bringing this to our attention.

Changes to this class will require a ton of testing (which you've already started 👍) to make sure there are no regressions in other situations. I'd also really like to understand the underlying issue.

joe-watkins commented 7 years ago

@scottaohara Aww interesting -- I'm sure that negative margin is there for a reason... can anyone remember?

@roblarsen tnx! :) Glad to help. Reading order is being affected by visual presentation some how. Very odd. It sure would be less heavy handed to look at the margin declaration per @scottaohara 's finds rather than changing position and adding display.

But, I'm wondering if it may be advantageous to have less absolutely positioned elements in the DOM? That a perf thing?

It feels silly to attack an Apple bug from this angle but we know how bug submissions and updates go with that camp.

roblarsen commented 7 years ago

@joe-watkins it's an OLD commit relating to issue #194, where there is a LOT of discussion of the original code. I'll dive in a bit later to check it out myself, but there's got to be some reasoning in there.

scottaohara commented 7 years ago

I assumed the negative margin was for legacy browsers (since the linked commit/issue reference ie7), as I have yet to find a situation where it's been useful/needed at all in ie9+ projects.

doing a quick check in ie7/8/9, i'm still trying to find exactly what that margin did... possibly a fall back to negate a stray pixel if the visuallyhidden content had a background color?

joe-watkins commented 7 years ago

@scottaohara Yes, after poking around a bit it looks like it is to combat single pixel appearing.

I wonder if this dark art would be considered:

.visuallyhidden {
  ...
  margin: -1px/9; /* ie8 and below */
  ...
}
scottaohara commented 7 years ago

while i am a fan of the occasional practicing of dark arts, @joe-watkins, the README states it supports ie9+

I haven't seen the pixel in ie9 plus, have you with your testing?

joe-watkins commented 7 years ago

@scottaohara Nor have I seen any issues in IE9... even by giving it a huge width/height and background color. clip is doing the magic with the visibility.

Removal of the negative margin sounds like the fix.

Thoughts?

roblarsen commented 7 years ago

We don't need to worry about old IE anymore, so if we can clean this up with the removal of the negative margin, then that's great. I'd love to see a PR for this. One last bug fix before I ship 6.0.

roblarsen commented 7 years ago

Closed via #1986

joe-watkins commented 7 years ago

Noticed issue persists when a line-height is defined. Continued testing needed.

https://github.com/h5bp/html5-boilerplate/pull/1986#issuecomment-322253065

scottaohara commented 7 years ago

my comment outlining additional options to rectify the bug

roblarsen commented 7 years ago

BUMMER.

The good news is, removing old IE code is still a valid thing to do these days.

joe-watkins commented 7 years ago

Hey all, so after some fiddling around with the help of @scottaohara in https://github.com/h5bp/html5-boilerplate/pull/1986 we have landed on a solution a bit more like what I originally was thinking with a twist..bringing back the negative margin which helps with visually oddities with focus ring, removing the absolute positioning, and introducing display: inline-block; and things appear to be working well.

I've introduced this snippet into existing projects and things hold up well in all browser/at combinations I can test (mentioned in original post). Reading order in Safari/Chrome/VoiceOver is correct.

It would be great if some others could run this through the ringer:

.visuallyhidden {
    border: 0;
    clip: rect(0 0 0 0);
    clip-path: inset(50%);
    display: inline-block;
    height: 1px;
    margin: -1px;
    overflow: hidden;
    padding: 0;
    width: 1px;
    white-space: nowrap; /* 1 */
}
joe-watkins commented 7 years ago

As pointed out by @tomasz1986 in https://github.com/h5bp/html5-boilerplate/pull/1986#issuecomment-322661868 the absolute positioning was structural for the clip for old browsers. Recommending removal of that.

.visuallyhidden {
    border: 0;
    clip-path: inset(50%);
    display: inline-block;
    height: 1px;
    margin: -1px;
    overflow: hidden;
    padding: 0;
    width: 1px;
    white-space: nowrap;
}

https://codepen.io/joe-watkins/pen/vJWMLp

With a green light I could submit a PR for this :)

roblarsen commented 7 years ago

What testing has been done? Is there any missing coverage? I'd like to put together a laundry list of what we've tested in.

joe-watkins commented 7 years ago

Hi @roblarsen below you will find the browsers/AT I've tested the latest CSS with and all is well.

SR/AT Tested (works as expected in)

Win IE11,IE10,FF,Chrome with JAWS 18/17 Win IE11 w/Dragon Naturally Speaking Win IE11,IE10,FF,Chrome NVDA macOS Seirra Safari,Chrome,FF VoiceOver iOS 10 VoiceOver / Safari Android 7 TalkBack / Chrome

Browsers tested (works - visually hides text in)

Win Edge, IE11, IE10, IE9, IE8, IE6, FF 53, Chrome 60 macOS Seirra Safari 10.2, Chrome 60, FF 54 iOS 10 VO / Safari

roblarsen commented 7 years ago

Thanks @joe-watkins, that's perfect. Let's PR this.

roblarsen commented 7 years ago

closed via #1989

svinkle commented 7 years ago

Bad news: without position: absolute the .visuallyhidden element adds space to the container.

Demo: https://codepen.io/svinkle/pen/LjQJbZ

/cc @joe-watkins @scottaohara @roblarsen

scottaohara commented 7 years ago

(╯°□°)╯︵ ┻━┻

back to work then!

svinkle commented 7 years ago

I mean, you could apply a work-around to the extra space by specifying a container width and height but that's not realistic. 😕

joe-watkins commented 7 years ago

@svinkle Ahh nuts! Looks like a code/whitespace thing: https://codepen.io/joe-watkins/pen/yovRzJ (hmmmff!)

roblarsen commented 7 years ago

This issue is like a see-saw.

svinkle commented 7 years ago

@joe-watkins Interesting…

@roblarsen I might suggest a roll-back on the latest merge until this is sorted. Having extra, visual space is worse than the read order bug, imo.

scottaohara commented 7 years ago

@svinkle, @roblarsen, @joe-watkins :: so as joe said, it's definitely related to the display: inline-block; causing extra white space.

initial testing of setting margin to margin: -1px -1px -1px -.25em; seems to be doing the trick right now: https://codepen.io/scottohara/pen/GvQYOe?editors=1100 (.25em being the '-4px' equivalent per one of the css-tricks fixes)

but would love additional hammering please.

edit: meh...seems to not be working as well as i had hoped....works ok in the text after example, but not the text before...the fa icon is display: inline-block as well, and even with the margin-right: -.25em on the visually hidden class, the fa inline block still generates its own extra white space that's muddling the spacing

joe-watkins commented 7 years ago

This is in no way a fix.. but does appear to address this case. float + the margin: -1px -1px -1px -.25em; Needs further testing.

https://codepen.io/joe-watkins/pen/YxeJJB?editors=1100

Perhaps we can't fix a Webkit/Blink bug from this angle and need to revert?

svinkle commented 7 years ago

float is causing more/different spacing issues on the site I'm working on…

I vote revert.

scottaohara commented 7 years ago

yeh. revert is probably the way to go, but humor me and give this a shot, @svinkle

.visuallyhidden-with-out-height {
    border: 0 !important;
    clip: rect(0 0 0 0);
    clip-path: inset(50%);
    overflow: hidden;
    width: 1px;
    position: absolute;
    white-space: nowrap;
}

it's from a previous test i was doing that causes a bump in the focus ring depending on how much styling is applied to the links/page. i'm not suggesting we go with it for html5bp, but would be curious to know how it fairs in that project you were testing in?

svinkle commented 7 years ago

@scottaohara No dice; reads out of order. 😕

scottaohara commented 7 years ago

time to re-file a bug with webkit :)

edit: similar issue reported - https://bugs.webkit.org/show_bug.cgi?id=173914

joe-watkins commented 7 years ago

👍 on revert.

joe-watkins commented 7 years ago

I've submitted two bugs. I know @svinkle has submitted one with Apple in the past.

Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=757177

Apple: Bug ID: 33979302

@roblarsen We may need to revert back to before #1986

scottaohara commented 7 years ago

Do we want to revert to before or after the removal of the negative margin?
margin: 0; clearly wasn't the fix we needed, but it did help for some use cases & didn't introduce any other bugs.

Thoughts?

svinkle commented 7 years ago

I'd say revert to abc64efcb71847049620bf8ae78f3cd5ce26bf5e. The class definition was quite stable at this point in time.

I can't say for certain, but pretty sure margin: -1px exists to offset the width: 1px and height: 1px properties.

roblarsen commented 7 years ago

I'm going to revert to the stable version later today

scottaohara commented 7 years ago

@svinkle right, it offsets it, but from what we've identified, that seems to be part of the issue, offsetting it outside of the focus ring area.

being that it's a position absolute element, the margin doesn't affect the DOM, the clip/clip-path & overflow are what actually hide the element. this just seems to be arbitrarily moving it, which may have had a purpose in older browsers, but other versions of the visuallyhidden class I've seen in practice / promoted by other CSS/a11y individuals, don't use this declaration at all anymore.

Apologies if this comes across as me being obstinate, that is not my intent and I agree reverting to a stable version is the way to go. I'm just trying to figure out what the benefit of this particular declaration is/was. I'll just keep testing :)

svinkle commented 7 years ago

@scottaohara Sounds good to me! If we're certain it serves no purpose and doesn't negatively effect the class, by all means. 👍

scottaohara commented 7 years ago

@svinkle @joe-watkins

I continue to battle against this... I've included multiple tests and am trying out various versions of the hiding content classes.

Presently I'm using the .at-only class that I often use myself and have modified a bit (current version in this codepen reads content in order, in the testing i'm currently doing) .visuallyhidden from html5bp (without margin) .snooka from https://snook.ca/archives/html_and_css/hiding-content-for-accessibility

please feel free to join me if you want to waste your time as well :)

KayLeung commented 7 years ago

As far as I can remember, the current code was fixed the width: 0; height:0 problem for VoiceOver @see: ref. I tested the latest MacOS and has no this issue. Is it the right time back to a simple workaround with ZERO value?

Could anyone test the older MacOS?

bradkemper commented 7 years ago

Has anyone tried margin: 0 -1px -1px 0? I think that would prevent offsetting content by the 1px width and height, without influencing the width and height of the container, nor the reading order. Assuming left to right reading.

afercia commented 7 years ago

For reference, similar issue in WordPress: https://core.trac.wordpress.org/ticket/42006 (Edit: anyways, it's a Safari/VoiceOver bug)

Malvoz commented 6 years ago

This is how Google AMP does it:

/**
 * Makes elements visually invisible but still accessible to screen-readers.
 *
 * This Css has been carefully tested to ensure screen-readers can read and
 * activate (in case of links and buttons) the elements with this class. Please
 * use caution when changing anything, even seemingly safe ones. For example
 * changing width from 1 to 0 would prevent TalkBack from activating (clicking)
 * buttons despite TalkBack reading them just fine. This is because
 * element needs to have a defined size and be on viewport otherwise TalkBack
 * does not allow activation of buttons.
 */
.i-amphtml-screen-reader {
  position: fixed !important;
  /* keep it on viewport */
  top: 0px !important;
  left: 0px !important;
  /* give it non-zero size, VoiceOver on Safari requires at least 2 pixels
     before allowing buttons to be activated. */
  width: 4px !important;
  height: 4px !important;
  /* visually hide it with overflow and opacity */
  opacity: 0 !important;
  overflow: hidden !important;
  /* remove any margin or padding */
  border: none !important;
  margin: 0 !important;
  padding: 0 !important;
  /* ensure no other style sets display to none */
  display: block !important;
  visibility: visible !important;
}
svinkle commented 6 years ago

Thanks for pointing this out, @Malvoz. Unfortunately the bug persists when testing with @joe-watkins's original pen here: https://codepen.io/joe-watkins/pen/OjpqxL 😕

These days I'm using aria-label more than .visuallyhidden elements if I can help it. Support is 💯 and ensures a properly formatted label in the accessibility tree.

joe-watkins commented 6 years ago

@Malvoz Interesting,.. I hadn't experimented much defining a size of 4px. Such a weird bug. I wish Safari wasn't such a walled garden.

@svinkle Dagnabbit! I can still get aria-label to fail in current Safari / VO on an anchor still when using only the up/down arrow keys for navigation. https://s.codepen.io/joe-watkins/debug/QMpPQQ

scottaohara commented 6 years ago

@joe-watkins yeh, Safari + VO don't respect ARIA as much when using up/down arrows to navigate content.

aria-labels and aria-hidden are not respected at all, just to name a couple. Up/down arrows allow me to exit the majority of modal dialog scrips I've come across :)

But back to the mention of width, if you recall when we were hammering on this, I found that if the width is not restricted, this issue can be partially mitigated.

afercia commented 6 years ago

@joe-watkins yeh, Safari + VO don't respect ARIA as much when using up/down arrows to navigate content.

aria-labels and aria-hidden are not respected at all, just to name a couple. Up/down arrows allow me to exit the majority of modal dialog scrips I've come across :)

Maybe because with Safari and VoiceOver you have to use Control-Option-Right Arrow and Control-Option-Left Arrow to navigate content? ;) aria-label and aria-hidden work perfectly for me and I haven't heard of any big issues with them.

scottaohara commented 6 years ago

That's one way to navigate content. but navigating line by line can be done with up/down arrows. so have to is not exactly an accurate statement there.

afercia commented 6 years ago

As well as "aria-labels and aria-hidden are not respected at all" :)

joe-watkins commented 6 years ago

@afercia Yes using VO keys things behave as expected, but if you don't use the VO keys and only the arrow keys aria-label fails.. so the alternative method for navigating is an option and there isn't a way to enforce one method of navigation over another. Quick nav behaves well which is good :)