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 form validation tooltip alignment #31557

Merged
merged 6 commits into from Oct 30, 2020

Conversation

domdomegg
Copy link

@domdomegg domdomegg commented Aug 31, 2020

Fixes #31507, as per my comment there (#31507 (comment))

Summary: IE is non-standards compliant about positioning. Fix was put in place which broke alignment in all browsers. This implements a more robust fix, and scopes it to IE only.

Tested in Google Chrome, Firefox, Edge, Internet Explorer 11, Google Chrome (Android).

Before After
image image
image image
(IE, before #30960) iebefore (IE, after this PR) ieafter

v4-dev: https://v4-dev--twbs-bootstrap.netlify.app/docs/4.5/components/forms/#tooltips
this branch: https://deploy-preview-31557--twbs-bootstrap.netlify.app/docs/4.5/components/forms/#tooltips

@domdomegg domdomegg requested a review from a team as a code owner August 31, 2020 13:22
@ffoodd
Copy link
Member

ffoodd commented Aug 31, 2020

Thanks for the diagnostis and PR!

I read your explanations carefully and tried another way: using left: auto instead of left: 0 solves the issue too since it's the initial position for the element —just tested in BrowserStack on Firefox, Chrome and IE11, works fine.

Could you try, please—and if you can confirm this, update your PR?

@domdomegg
Copy link
Author

Thanks ffoodd for reviewing so quickly! 😃

I believe left: auto causes IE to misbehave as in #25511 when the validation tooltip is within a .input-group. Something about that setup makes IE fail to properly position the absolute element.

ieproblem

Snippet to reproduce in IE

(taken from the Bootstrap 4.0 docs)

<form class="needs-validation" novalidate>
  <div class="form-row">
    <div class="col-md-4 mb-3">
      <label for="validationTooltip01">First name</label>
      <input type="text" class="form-control" id="validationTooltip01" placeholder="First name" value="Mark" required>
      <div class="valid-tooltip">
        Looks good!
      </div>
    </div>
    <div class="col-md-4 mb-3">
      <label for="validationTooltip02">Last name</label>
      <input type="text" class="form-control" id="validationTooltip02" placeholder="Last name" value="Otto" required>
      <div class="valid-tooltip">
        Looks good!
      </div>
    </div>
    <div class="col-md-4 mb-3">
      <label for="validationTooltipUsername">Username</label>
      <div class="input-group">
        <div class="input-group-prepend">
          <span class="input-group-text" id="validationTooltipUsernamePrepend">@</span>
        </div>
        <input type="text" class="form-control" id="validationTooltipUsername" placeholder="Username" aria-describedby="validationTooltipUsernamePrepend" required>
        <div class="invalid-tooltip">
          Please choose a unique and valid username.
        </div>
      </div>
    </div>
  </div>
  <div class="form-row">
    <div class="col-md-6 mb-3">
      <label for="validationTooltip03">City</label>
      <input type="text" class="form-control" id="validationTooltip03" placeholder="City" required>
      <div class="invalid-tooltip">
        Please provide a valid city.
      </div>
    </div>
    <div class="col-md-3 mb-3">
      <label for="validationTooltip04">State</label>
      <input type="text" class="form-control" id="validationTooltip04" placeholder="State" required>
      <div class="invalid-tooltip">
        Please provide a valid state.
      </div>
    </div>
    <div class="col-md-3 mb-3">
      <label for="validationTooltip05">Zip</label>
      <input type="text" class="form-control" id="validationTooltip05" placeholder="Zip" required>
      <div class="invalid-tooltip">
        Please provide a valid zip.
      </div>
    </div>
  </div>
  <button class="btn btn-primary" type="submit">Submit form</button>
</form>

@ffoodd
Copy link
Member

ffoodd commented Sep 2, 2020

Aww you're right, thanks -_-

So back to your suggestion: we usually try to avoid CSS hacks, this is why we simply added left: 0 at first glance without any hack to target IE.

Wouldn't your suggestion work if you simply used .form-row > .col > &, .form-row > [class*="col-"] > & { … } directly? It'd be easier to read and understand, and would involve less code.

Any opinion, @twbs/css-review?

@domdomegg
Copy link
Author

Yep that should work in almost all cases, it's just in the cases where the tooltips are added to some other element with different amounts of padding that it might be misaligned - this would happen in IE, but thought a CSS hack could restrict that to IE and have other browsers display normally.

Don't mind either way, happy to make it the behaviour in all browsers - as I said, in probably almost all cases it would work (and either way would be better than what we have now)

@domdomegg
Copy link
Author

Hi @ffoodd, sorry to bother you again - has some kind of consensus been reached about this yet? :)

@ffoodd
Copy link
Member

ffoodd commented Sep 11, 2020

No one stepped in yet, however I'd recommend to drop the hacks since use them.

@mdo
Copy link
Member

mdo commented Sep 14, 2020

Agreed on hacks—we don't use CSS hacks in general unless absolutely necessary.

@domdomegg
Copy link
Author

domdomegg commented Oct 4, 2020

Sorry for the delay, should be updated now @ffoodd :)

Netlify preview

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

LGTM. @mdo @MartijnCuppens any comment?

Copy link
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

I'm good with this! To confirm, we don't need to do this in v5, correct? Could be nice to make a note of that somewhere (perhaps just in the blog post?).

@XhmikosR XhmikosR added this to Inbox in v4.6.0 via automation Oct 19, 2020
@XhmikosR
Copy link
Member

BTW shouldn't we move the button a little lower so that the tooltip doesn't overlap with it? Just thinking out loud, probably affects both branches.

@XhmikosR
Copy link
Member

@ffoodd @MartijnCuppens should we move the button a little lower with margin top? This affects v5 too IIRC.

@ffoodd
Copy link
Member

ffoodd commented Oct 30, 2020

I don't think so:

  1. the same issue happens with labels above,
  2. those are tooltips meant to help, it makes sense to have them more visible than the underlying form.

I'd say no, but that's highly debatable.

@XhmikosR
Copy link
Member

I personally find it very annoying that the validation feedback overlaps but maybe it's just me. :)

@XhmikosR XhmikosR merged commit 71e5b7c into twbs:v4-dev Oct 30, 2020
@XhmikosR XhmikosR moved this from Inbox to Shipped in v4.6.0 Oct 30, 2020
@ffoodd
Copy link
Member

ffoodd commented Oct 30, 2020

IMHO the problem is inherent to absolutely-positionned things above a fine tuned layout: you'd never have room for it. I personnaly recommend to not use tooltips, so… :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v4.6.0
Shipped
Development

Successfully merging this pull request may close these issues.

None yet

5 participants