Support from Acquia helps fund testing for Drupal Acquia logo

Comments

attiks created an issue. See original summary.

iamDamon’s picture

Assigned: Unassigned » iamDamon
iamDamon’s picture

Assigned: iamDamon » Unassigned
Status: Active » Needs review
FileSize
451 bytes

Following patch fixes the issue.

darketaine’s picture

It looks fine.

attiks’s picture

#4 If it's ok, you may set it to RTBC

iamDamon’s picture

Fix for Bartik

iamDamon’s picture

And here's the patch

attiks’s picture

Status: Needs review » Needs work

Patch has some side effects

darketaine’s picture

Status: Needs work » Needs review
FileSize
749 bytes

True, I think the problem isn't on the width of input, but on how browsers manipulate fieldset.

darketaine’s picture

Firefox needs another fix (display: table-cell;)

thpoul’s picture

FileSize
76.51 KB
89.04 KB
104.78 KB
89.29 KB

Looks fine by me in both themes.

thpoul’s picture

Component: link.module » Seven theme

Component is wrong, it should be either Seven Theme or Bartik Theme, tagging accordingly.

attiks’s picture

Status: Needs review » Reviewed & tested by the community

Looks good

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs subsystem maintainer review

I'd like a +1 from either @LewisNyman or @emma.maria, considering this is adding a browser-specific work-around.

darketaine’s picture

Issue tags: +Bartik

As this is on Seven theme component, Bartik is "getting lost" so I added the relative tag.

darketaine’s picture

Component: Seven theme » theme system
Issue tags: -Bartik

Actually as this is on both Bartik and Seven maybe the component should be changed

star-szr’s picture

Component: theme system » CSS
Issue tags: +frontend, +Bartik, +Seven

I don't think we have a CSS maintainer currently but theme system doesn't seem quite right either.

Does this also affect themes using Classy as their base? Not saying we should try to fix Classy necessarily but I'm curious.

darketaine’s picture

The patch is specific to Bartik and Seven as I think it will be a bit 'risky' to apply it in Classy, but yes, if that change came from Classy (I checked that it has no fieldset-specific rules) then all themes based on it would be 'fixed'.

prateekS’s picture

FileSize
9.91 KB

I have tested this patch in Seven, Bartik and Stark theme and one contributed theme(adminimal_theme) too. Its working fine in all of them(have added screenshot for this contributed theme). I think we should close it.

darketaine’s picture

This patch affects only Seven and Bartik (Adminimal is based on Seven) and no other theme (not even Stark).

emma.maria’s picture

Assigned: Unassigned » emma.maria

I will take a look at this issue for Bartik plus advise on Seven also.

emma.maria’s picture

Version: 8.0.x-dev » 8.1.x-dev
darketaine’s picture

I can see that the browser specific prefix causes a frustration.
The only reason it's added like that and not in fieldset { } it's because of

@-moz-document url-prefix() {
  .fieldgroup {
    display: table-cell;
  }
}

in /core/themes/seven/css/components/form.css. So I thought that's the way it should be done in context of consistency.
If it should be removed I guess it should be removed wherever it is used in core.

emma.maria’s picture

Assigned: emma.maria » Unassigned
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs subsystem maintainer review

Mozilla are actively trying to remove the @-moz-document browser hack...
https://bugzilla.mozilla.org/show_bug.cgi?id=1035091

But they are aware that it is popularly used to help add a FF specific fix to fieldset widths and they have set this bug as a blocker for removing the browser hack.
https://bugzilla.mozilla.org/show_bug.cgi?id=504622

We are using this exact hack right now in the CSS and I think it's OK that we further add it to anything else that needs it for now.

I'm also raising a follow up as Bartik has a different field (email with multiple values) which uses <table> markup as it also causes a horizontal scrollbar.

emma.maria’s picture

Issue tags: +Novice, +DrupalCampES

Comments need to be added above any @-moz-document selectors in the patch to say that this CSS will not be needed and removed once this Mozilla bug is fixed — https://bugzilla.mozilla.org/show_bug.cgi?id=504622.

thpoul’s picture

Status: Needs work » Needs review
Issue tags: -DrupalCampES
FileSize
1.52 KB
1.71 KB

Here you go.

Maninders’s picture

Assigned: Unassigned » Maninders
Maninders’s picture

FileSize
58.72 KB

This is working fine after applying patch #26. Attached screenshot for your reference(link-field.png).

Maninders’s picture

Assigned: Maninders » Unassigned
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Assigned: Unassigned » star-szr

Assigning to @Cottser as he has already been involved.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 2611268-26.patch, failed testing.

The last submitted patch, 26: 2611268-26.patch, failed testing.

emma.maria’s picture

Status: Needs work » Reviewed & tested by the community

The patch still applies cleanly...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 2611268-26.patch, failed testing.

star-szr’s picture

Status: Needs work » Reviewed & tested by the community

Migrate UI tests again as far as I can see. I sent for a retest yesterday so trying again.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 2611268-26.patch, failed testing.

star-szr’s picture

Status: Needs work » Reviewed & tested by the community

Migrate UI. Hoping to review this tomorrow or Sunday!

Maninders’s picture

star-szr’s picture

Didn't quite get to reviewing this - had a great workshop/discussion on Sunday at the Frontend United sprints though :)

  • Cottser committed 3c7e318 on 8.2.x
    Issue #2611268 by darketaine, iamDamon, thpoul: Link field on node edit...
star-szr’s picture

Version: 8.1.x-dev » 8.2.x-dev
Assigned: star-szr » Unassigned
Status: Reviewed & tested by the community » Fixed
Issue tags: -Novice

This is yucky to have to add more of these hacks but luckily it's temporary and it's something we should fix :)

Committed 3c7e318 and pushed to 8.2.x. Thanks!

Status: Fixed » Closed (fixed)

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