Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dcrocks’s picture

FileSize
168.51 KB

Here is a snapshot.

tim.plunkett’s picture

Issue tags: +Blocks-Layouts, +Block plugins

Tagging.

webchick’s picture

Issue tags: +Novice

While I don't know for sure, I'm going to guess that this is something silly and would be fairly easy for someone who knew their way around CSS.

irk’s picture

Sorry to just zoom by and drop in a link, but I'm between tasks right now and don't have the time to set up d8 on my comp just this instant to truly poke at this issue. However, to the person that is available to look at this sooner than I can, the link here may assist you:

http://stackoverflow.com/questions/52563/css-textbox-to-fill-parent-cont...

Happy bug hunting! If I check back when I am free and this is still open, I'll see what I can do then.

RobW’s picture

I don't have a copy of D8 spun up right now, but I'm going to bet a max-width: 100% on input elements would fix that right up.

thijsvdv’s picture

Any live testsite available?

dead_arm’s picture

Status: Active » Needs review
FileSize
439 bytes

Add a max-width of 100% to the input.

steve.colson’s picture

Status: Needs review » Reviewed & tested by the community

Confirming #8 fixes the issue in FF 18, Chrome 24, and Safari 6 on OS X.

ultimateboy’s picture

Status: Reviewed & tested by the community » Needs work

The max-width does fix the issue, however I have an issue putting this within the ".js" selector. The form still breaks without javascript.

webchick’s picture

Status: Needs work » Needs review

Per request :) here is a D8 demo site that can be used to debug further:

http://spark.webchick.net/cssfix/

Username / password: a / a

Then go to http://spark.webchick.net/cssfix/#overlay=admin/structure/block/list/blo...

...and I see that in the meantime a patch has been posted. ;) One sec, I'll test it.

webchick’s picture

Status: Needs review » Needs work

Oops! Cross-post.

thijsvdv’s picture

Even a simple width: 100%; on the input does the trick as there is no width defined.

But for a more general solution, adding max-width: 100%; on the top-level "input" selector could prevent future similar problems. Don't think that would interfere with anything as an input field shouldn't break out of its parent form I guess?

tim.plunkett’s picture

Before:
Screen Shot 2013-01-16 at 1.40.22 PM.png

After:
Screen Shot 2013-01-16 at 1.38.46 PM.png

After, no JS:
Screen Shot 2013-01-16 at 1.39.35 PM.png

ultimateboy’s picture

Status: Needs review » Reviewed & tested by the community

Alright. I'll RTBC that one :D

steve.colson’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
470 bytes

Question for the peanut gallery:

Would it be better to have it specific to auto-complete elements, or form elements in general? It seems like this may just be a style issue with the seven theme hypothetically affecting other elements that we may not be seeing yet. As an alternative, I suggest moving the max-width out of core blocks and over to seven so we don't need to distinguish element type or js/no-js...

dead_arm’s picture

The issue occurs in both seven and bartik, so it should be in system.

steve.colson’s picture

Hmm. Didn't mean to pull it out of rtbc. I am guessing posting a second patch does that? If so, sorry.

Regardless, I would be curious what others think about the right spot for this.

steve.colson’s picture

dead_arm:

Ok. Then what about putting it in .form-item input instead of just autocomplete?

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

#13 is RTBC.

peruvianidol’s picture

The input itself has a size attribute of 60 set on it. I would remove this and apply "width: 100%" to the input in the CSS.

babbage’s picture

@peruvianidol: width: 60px; max-width: 100% is not the same as width: 100%. Not saying that your solution is necessarily invalid, but in a responsive design world there are scenarios where the search box would end up much wider than 60px with the latter.

babbage’s picture

From #12:

But for a more general solution, adding max-width: 100%; on the top-level "input" selector could prevent future similar problems. Don't think that would interfere with anything as an input field shouldn't break out of its parent form I guess?

This sounds like a good idea to me. The issue is, it would have hugely wide-ranging effects and so would be hard to properly test as there presumably aren't tests for this? Seems to me this would also be increasingly important as part of getting a core theme/core themes to be responsive. That said, could be worth a punt here, or as a follow-up issue?

RobW’s picture

Re: max-width: 100% on input elements:

This is exactly what should happen. It

  • Fixes this probable display problem everywhere, automatically.
  • Doesn't break any width a theme developer would specify unless they specifically want an input to extend outside of its container. Amount of times this is likely to happen when the developer is not already prepared to deal with some oddness: 0.
  • Has wide ranging effects, but they're all positive/corrective.
  • Unexpected or frustrating behavior that could result from this: none that I can think of.

The problem is exacerbated by the size attribute (and removing it isn't a fix because without it set there is a default of 20 for text inputs). Sitepoint advocates deprecation of the size attribute, MDN makes no judgement. IIRC it was required in html4/xhml, but doesn't look like it is for HTML5.

I would put the rule in system, or in a base form css file if D8 has one. Sorry I don't have time to actually write the patch.

webchick’s picture

Title: Dangling search box after blocks-as-plugins » Form elements can break out of their bounding divs and whatnot
Component: block.module » CSS
Status: Reviewed & tested by the community » Active
Issue tags: -Blocks-Layouts, -Block plugins

So as a wee small-brained PHP developer, I find this conversation very confusing. :)

It sounds like #13 fixes the bug with the dangling search box on this particular form. Hooray!
It sounds like #15 / #21 / #22 is probably a more correct fix, but may need more discussion / testing.

So here's what I'm going to do. For now:

Committed and pushed #13 to 8.x. Thanks!

...because now I can stop swearing every time I go to the blocks page. ;) However, I'll now re-title/re-orient this issue around fixing the problem generally, so we can take some time to evaluate that. Still leaving as Novice because it seems like this would be relatively easy to make a patch for. Anyone up for it?

Also, for completeness, here were some comments that were on Twitter but didn't make their way to the issue queue:

https://twitter.com/csixty4/status/291611064079114241
@webchick Looks like Firefox is respecting the size attribute on the text input. @meyerweb got a good cross-browser way to override?

https://twitter.com/meyerweb/status/291618269864026112
@webchick Yeah, the proposed solution should be fine. Not clear why there’s a 'size' attribute on the input, though—seems unnecessary.

babbage’s picture

Assigned: Unassigned » babbage

Seems to me that there may be a number of other input elements that already have max-width: 100% set to fix this same problem elsewhere. So it's possible that by setting this as the default for all input elements, we may be able to remove quite a few element-specific CSS rules across core. If so, that'd be a nice win. I'm happy to work on a patch in the next couple of days if someone else isn't already working on this. Tentatively assigning to me, but happy for someone to grab this back if they're already halfway through a patch.

echoz’s picture

Status: Active » Needs review
FileSize
670 bytes

A text input or textarea, to fill a container's width, needs an explicit 100% width (or they'll be the width of the cols attribute (this is characters, not px). I find max-width to work almost the same, except in a wider viewport, the text input will stop expanding at the cols attribute width, but width: 100%; keeps it at 100% both narrower + wider.

In addition, when an explicit width is set, it will also need box-sizing: border-box; or any side padding and/or border will add to the width, pushing it out of the parent container. Notice in the current fix, the elememt pushing more to the right.

In the past few weeks, I've looked at a lot of admin forms where this is needed and works without conflict. Lower in this file we see the same for the textarea. We might be able to remove the element selector (input).

RobW’s picture

The class should definitely not be qualified by the input element. It causes specificity problems, all sorts of overrides needed if the html structure is changed, is just bad practice, etc.

Instead of dropping the element, it's probably a better idea to drop the class. We're don't just need to fix the overflow for all .form-texts, we need to fix it for all input elements.

steve.colson’s picture

FileSize
724 bytes

I would generally disagree with setting a forced 100% width on all input elements everywhere as that will likely mess up lots of things all over. Rerolled for just input as the selector using max-width.

echoz’s picture

Agreed, my comment about writing it without the element selector was about the specificity problems, which we are planing on cleaning up. I wanted to patch existing and admittedly didn't have the time to test, so kept the qualified selector that is used frequently.

It seems we would use class .form-text because input would include checkbox and radio. How's this?

echoz’s picture

FileSize
670 bytes

Yikes, 2 secs.

babbage’s picture

From #26:

A text input or textarea, to fill a container's width, needs an explicit 100% width (or they'll be the width of the cols attribute (this is characters, not px).

I haven't looked at the patch yet, but just to say this is definitely not what this patch is intended to achieve. We are not trying to force inputs to be 100% wide, we are only trying to prevent them being wider than their parent container. It sounds from the subsequent discussions like this has been moved away from, but for the sake of clarity for any later readers I wanted to state this explicitly.

tim.plunkett’s picture

Status: Needs review » Needs work

Agreed with @babbage.

echoz’s picture

Status: Needs work » Needs review

#31 point taken, but at 100% width, they line up with textareas which are 100% width, and are ready for responsive narrow viewports in which most themes change them to 100% width. Also, as seen in #29 screenshots, they won't stop expanding at the cols attribute width on a wider viewport.

echoz’s picture

Status: Needs review » Needs work

didn't mean to change your status change.

babbage’s picture

@echoz: the purpose of this issue is to provide a fix for form elements that can break out of their bounding divs.

If you want to propose that all input areas should be 100% wide, then you would need to create a separate issue proposing that as it is an entirely separate issue—and that is a pretty radical design proposal to force as a default, so it is almost certain to not get support I'm afraid. :)

echoz’s picture

Ok, agreed on max-width. Have been writing too much narrow viewport css, sorry for the noise!

steve.colson’s picture

Status: Needs work » Needs review

Ok, so I think the latest "for review" is #28. It implements the suggestions from #27 and keeps it at max-width rather than forced width.

echoz’s picture

FileSize
692 bytes

The last up for review was mine at #30 (and #28 failed testing), so I'll fix.

Status: Needs review » Needs work

The last submitted patch, blocks-1880368-38.patch, failed testing.

echoz’s picture

The 2 patches with "input" as the selector, failed testing, but with different reasons. What is the next step?

webchick’s picture

That seems quite dubious. Sending for a re-test.

webchick’s picture

Status: Needs work » Needs review
Issue tags: -Novice

#38: blocks-1880368-38.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice

The last submitted patch, blocks-1880368-38.patch, failed testing.

steve.colson’s picture

FileSize
673 bytes

Echoz, I'm confused what is different in your patch on #38 than mine on #28?

(reposting for retesting)

steve.colson’s picture

Status: Needs work » Needs review

retesting needs review status

echoz’s picture

#44, I didn't think it was different, but when you wrote in #37 "Ok, so I think the latest "for review" is #28" and only changed the status to "needs review", I saw that the latest patch was in fact mine in #30 and besides, #28 had failed testing, so the review would have been for mine which we had decided was not ok. Not knowing we could just retest yours at that point, I just corrected and resubmitted my patch.

Status: Needs review » Needs work

The last submitted patch, blocks-1880368-28.patch, failed testing.

steve.colson’s picture

Status: Needs work » Needs review
FileSize
724 bytes

Reroll patch from latest 8.x HEAD to try and fix test errors.

echoz’s picture

This would be RTBC for me if the explanatory comment were more accurate. "Keep form elements from breaking the box." seems unclear. I'd rather have something like "breaking out of their container" or "overflowing their container". Apologies for nit picking!

steve.colson’s picture

FileSize
736 bytes

Good call. Comment is now more non-designer friendly.

Status: Needs review » Needs work

The last submitted patch, blocks-1880368-50.patch, failed testing.

jthorson’s picture

Bot problem. Has been requeued.

RobW’s picture

There should probably be a follow up issue deciding on where to apply border-box. At the least, if it's added to inputs it should be added to texareas.

This patch looks good though.

echoz’s picture

@RobW, as I noted in #26, lower in system.base.css we see textarea (.form-textarea-wrapper textarea) has box-sizing: border-box; set.

steve.colson’s picture

Status: Needs work » Needs review

#50: blocks-1880368-50.patch queued for re-testing.

echoz’s picture

Status: Needs review » Reviewed & tested by the community

RTBC it is!

RobW’s picture

Echoz, thanks, missed that!

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Rock! Thanks so much for this, folks!

Can I get some confirmation from people on which forms they tested, which browsers, etc? I get nervous on global classes like this and want to make sure some of our more complex UIs (e.g. Views UI, manage fields, inline editing) continue to work.

steve.colson’s picture

Webchick,

Noted and screenshots forthcoming in a bit.

echoz’s picture

Just marked #1879966: User login, register, forgot password in viewport or column as a duplicate of this issue. This patch fixes that issue, and once again brings up the need for the more general element selector, input.

Shyamala’s picture

Added tags

echoz’s picture

FileSize
44.32 KB
44.44 KB
44.71 KB
44.41 KB

As requested, webchick...
Most text fields from within admin already have this covered by Seven's width 100% (and box-sizing: border-box;) when in narrow viewport. This issue could be spotted in non admin narrow screens (screenshots attached), or in a column narrower than the form element's size attribute (this original issue). So as I clicked around, it was hard to find other screenshots :-)

After shots are with #50 patch applied.

echoz’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Assigned: Unassigned » webchick
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Wow, I'm really sorry! This somehow dropped completely off my radar! I blame Sydney and vacations. :)

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.