Closed Bug 140562 Opened 22 years ago Closed 8 years ago

Replace -moz-focus-inner from form elements with standard outline

Categories

(Core :: Layout: Form Controls, defect, P1)

Product:
Core
Component:
Layout: Form Controls
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Future
Tracking Flags:
Tracking Status
blocking-fx --- -
firefox53 --- fixed

People

(Reporter: jens.schiffler, Assigned: wisniewskit)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, testcase, Whiteboard: workaround in comment 14)

Attachments

(9 files, 4 obsolete files)

testcase
357 bytes, text/html
Details
testcase from bug 208896
2.66 KB, text/html
Details
testcase 3
328 bytes, text/html
Details
left: wrong firefox focus-ring, right: native focus-ring
2.25 KB, image/png
Details
Patch v1
1.37 KB, patch
bzbarsky
: feedback+
Details | Diff | Splinter Review
testcase: default button padding
1.70 KB, text/html
Details
140562-1-remove_unused_moz_focus_outer_code_for_buttons.diff
16.01 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
140562-2-remove_2px_padding_on_buttons_for_moz-focus-inner_ring.diff
21.99 KB, patch
Details | Diff | Splinter Review
140562-3-fix_up_reftests.diff
37.59 KB, patch
Details | Diff | Splinter Review 
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0rc1)
Gecko/20020417
BuildID:    2002041711

a "button" element with zero width border and zero width padding is still
displayed with a border surrounding its contents when it contains a "table" element.

Reproducible: Always
Steps to Reproduce:
1. Create a html document with the snippet below
2. Open the document with Mozilla

Actual Results:  The button has a blue border, the table gets clipped.

Expected Results:  The button should have no border at all, instead only the
contained element should be shown.

<button
style="border:0px;padding:0px;background-color:#00f;width:300px;height:50px;">
<table 

style="width:300px;height:50px;background-color:#ffff00;border:1px;border-style:solid;border-color:#000;>

<tr>
<td>A field</td>
<td>Another field</td>
</tr></table>
</button>
Attached file testcase — 

    
    

    
  
Confirming bug -- the button still has padding...
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All

    
  
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Future

    
    

    
  
*** Bug 183335 has been marked as a duplicate of this bug. ***

    
    

    
  
the only way to solve this is to fix our focus rect stuff...

    
    

    
  
Conirmed in 1/03/03 Trunk build, Linux. Testcase keyword added.
Keywords: testcase

    
    

    
  
*** Bug 208896 has been marked as a duplicate of this bug. ***

    
    

    
  


  
testcase with <a> element within <button> element

    
    

    
  
A workaround is to set "::-moz-focus-inner" to have a 0px padding and border. This will hide the extra padding, but will also cause the button to lose it's focus ring. To workaround that, set "button:focus" to have an outline that is placed around the outside of the button. See here:

<html>
<style type="text/css">
button {padding:0; border:0}
button::-moz-focus-inner {padding:0; border:0}
button:focus {outline: 1px dotted}

table {background:yellow; border:1px solid}
</style>
<button>
<table><tr><td>button1</td></tr></table>
</button>
</html>

    
    

    
  
Probably it make sense to say that the bug appears not only when button contains table, but also when button contains other elements like span and also when there are not elements inside button at all.

Fx 2.0.0.7, Fx 3 (2007092405 Minefield/3.0a9pre).

    
    

    
  
Note that we use the horizontal padding to do the "pressed" look as well.  See bug 179230.

    
    

    
  
There is another test case in bug 355849.
Assignee: rods → nobody
Status: ASSIGNED → NEW
QA Contact: tpreston → layout.form-controls

    
    

    
  

      
      
      
      

        Attached file
          
        testcase 3
        — 
      

    


  
workaround (from bug 573978)) is
::-moz-focus-inner { padding:0 }
::-moz-focus-inner { padding:0; border:0 }  /* ! removes focus line */

    
  
Summary: button control with 0px padding still displays a frame around its contents → <button> ignores CSS style padding:0
Whiteboard: workaround in comment 14

    
    

    
  
focused buttons have, compared to non-focused ones, a special look on every os (e.g. on window vista/7 it’s a cyan inset boxshadow).

so we can securely remove the ::-moz-focus-inner element and instead give button:focus an effect reminiscent of the native one on the respective os when they are styled (remember: buttons look native except you change border or background). if web designers are not happy with that, nobody prevents them from changing it.

    
    

    
  
sorry for spamming, but even the following hack seems to be much better than the current pseudoelement:

button:focus {-moz-box-shadow: 0 0 2px -moz-html-CellHighlight inset}

and guys/gals: this bug is 8 years old. it’s really time to fix it!

    
  
Summary: <button> ignores CSS style padding:0 → Remove -moz-focus-inner from form elements

    
  
Summary: Remove -moz-focus-inner from form elements → Replace -moz-focus-inner from form elements with standard outline

    
  
Blocks: 435697
No longer blocks: 435697
Blocks: 418521

    
  
blocking-fx: --- → ?

    
    

    
  
I haven't contributed to the Mozilla project before, so I'd like to have a go at fixing this for my first bug (it only involves removing a few lines from the UA form.css stylesheet, so it should be easy).

I've downloaded MozillaBuild, but Mercurial won't open. Where's a good place in mozilla to go to get help with this?

    
    

    
  
(In reply to comment #25)
> I've downloaded MozillaBuild, but Mercurial won't open. Where's a good place in
> mozilla to go to get help with this?

Josh, I believe you should use Mercurial as 'hg' on the command prompt.
You should come over #developers on IRC (irc.mozilla.org), see: http://irc.mozilla.org/ if you want quick answers to your questions. Feel free also to ask some questions in the appropriate groups listed here: http://www.mozilla.org/about/forums/

    
    

    
  
Another great resource is MDN (mozilla developer network). Information on building Firefox is available here: https://developer.mozilla.org/En/Developer_Guide/Build_Instructions

    
  
blocking-fx: ? → -

    
    

    
  
So, what would be the fix for this? Is removing the ::-moz-focus-inner styles altogether and replacing them with :-moz-focusring styles on the button/input element the way to go? Or is this inner pseudo-element still necessary?

This is what I've published today in a page about styling button elements:

/* Firefox fix - bug 140562
   We're using :-moz-focusring rather than :focus so that we don't
   change the default focus in other browsers.
*/
button::-moz-focus-inner {
  padding: 0;
  border: none;
}
button:-moz-focusring {
  outline: 1px dotted;
}
Blocks: 654225

    
    

    
  
It's really hard to answer the question from comment 28 without a clear description of what we're trying to do.

Last I'd checked, at least on Windows the focus outline needed to go around the text inside the button.  That's not possible to do usefully with an outline style on the button itself, because you have to try to guess the offset from the button edge to the text, which is variable.

But maybe the currently desired look is different?  If someone actually familiar with the desired UI description across all supported OSes can write down what the exact desired effect is here, both with and without native theming, then we can have some sort of sane discussion about the actual code changes needed.
Keywords: uiwanted

    
    

    
  


  
last time I checked, the focus-ring on native buttons isn’t around the text, but around the inside of the button.

on the attached file you can see how firefox’ current behavior is wrong and native elements should look instead.

my proposal is to use a value lower than the default padding as default negative outline-offset, so sth. like button:focus {outline:1px solid black; outline-offset:-3px} should work well enough, some tweaking notwithstanding.

    
    

    
  
I think this is a bit too low-level for the UX team to have any specific opinion about. Sorry!
Keywords: uiwanted

    
    

    
  
We discussed with Boris about that and using a testcase [1], it seems that only Windows is using focus-inner and IE9 is actually using an outline that is a bit inside the element instead of a focus-inner.
So, the plan would be to not use the inner any more and use the outline for Windows only. That should fix the issue we are seeing here.

[1] http://oldworld.fr/mozilla/focus-inner.html

    
  
Keywords: dev-doc-needed

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v1
        — 
          — Splinter Review
      

    


  
It's taken me over a year, but I've finally made my first patch. It's probably wrong, though, so I've submitted it for feedback. Although I wasn't entirely sure who to ask for it.

        Attachment #663203 -
        Flags: feedback?(mounir)

    
    

    
  
Comment on attachment 663203 [details] [diff] [review]
Patch v1

Review of attachment 663203 [details] [diff] [review]:
-----------------------------------------------------------------

I doubt this patch is enough but I would have to do some researches. Boris should know that on top of his head.

        Attachment #663203 -
        Flags: feedback?(mounir) → feedback?(bzbarsky)

    
    

    
  
Comment on attachment 663203 [details] [diff] [review]
Patch v1

This is a good start, though "-3px dotted ButtonText" is not a valid value for "outline".  You probably want something more like this:

  outline: 1px dotted ButtonText;
  outline-offset: -3px;

And that said, what happens when the border styling on the button is changed?  outline-offset measures from the outer border edge, so giving the buttons a 5px border might screw up the outlines.  :(

Also, once we do this the code in nsHTMLButtonControlFrame::Reflow that looks at focusPadding can go away.  So can a bunch of methods on nsButtonFrameRenderer.  So can the Get/SetAdditionalStyleContext methods on nsHTMLButtonControlFrame.

        Attachment #663203 -
        Flags: feedback?(bzbarsky) → feedback+

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #36)
> Comment on attachment 663203 [details] [diff] [review]
> Patch v1
> 
> This is a good start, though "-3px dotted ButtonText" is not a valid value
> for "outline".  You probably want something more like this:
> 
>   outline: 1px dotted ButtonText;
>   outline-offset: -3px;
> 
> And that said, what happens when the border styling on the button is
> changed?  outline-offset measures from the outer border edge, so giving the
> buttons a 5px border might screw up the outlines.  :(

Thanks for the quick response. I can't believe I made a simple mistake like that in the syntax. In that case, would it be possible to apply the outline to the ::-moz-focus-inner pseudo-element. Shouldn't that overlay the outline above the inner edge of the padding box without creating a gap?

But if you'd rather not use that, would it be so bad to have the outline rendered outside the element? I know it'd be inconsistent with the native OS rendering, but it might be more ideal than the current situation of having the buttons laid out with different dimensions to what is set in a Web page's stylesheet.

    
    

    
  
> In that case, would it be possible to apply the outline to the ::-moz-focus-inner
> pseudo-element.

You can, yes.  Then you'd need to write some C++ code to render it.  The ::-moz-focus-inner pseudo-element doesn't actually exist.  It's just there as a hook to have the buttonframerenderer look up its styles and do whatever drawing it wants to do.

Maybe this is the right approach to solving the border width thing, yes.

> would it be so bad to have the outline rendered outside the element?

I think so, yes.  It would really confuse users.

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #38)
> You can, yes.  Then you'd need to write some C++ code to render it.  The
> ::-moz-focus-inner pseudo-element doesn't actually exist.  It's just there
> as a hook to have the buttonframerenderer look up its styles and do whatever
> drawing it wants to do.
> 
> Maybe this is the right approach to solving the border width thing, yes.

In that case, I can't help. My skills only go as far as markup and CSS. I'm starting my first year at a software engineering course tomorrow, so maybe one day I'll be able to help with that, but right now there's not much more I can do. Sorry.

    
    

    
  
but we don’t want to keep the ::-moz-focus-inner

it’s intransparent, and a huge element of surprise for webdevs who want to control how focus looks.

if we have a inspectable style on the button element itself, whough, it’s easily discoverable and changeable.

---------------------

i think it’s OK if changing the border size changes where the focusring is. a changed border also makes the button appear nonnative, with an old bevelled border style and flat background color. this does nothing if not indicate that that as soon as (s)he’s changed that one, the way the button looks is now solely the web developer’s responsibility.

    
    

    
  
> but we don’t want to keep the ::-moz-focus-inner

That depends on who "we" is.

> i think it’s OK if changing the border size changes where the focusring is.

No, it's really not, in the way it would work here.  It looks completely fucked up when you do it that way.  Just try it (which I assume you haven't, based on the comment).  And since other UAs don't have that behavior, web devs won't expect it and will code things that lead to invisible focus rings.  At which point you have a serious accessibility problem.

    
    

    
  
hmm, i agree that invisible focus rings shouldn’t happen until the webdev seriously decided to do so.

but you overlooked one part of my post: if one changes the border size, and nothing else, it *will* look completely **** up, no matter if it is focused or not. it will look like a windows-95 bevelled abomination.

chrome doesn’t have that problem because it doesn’t give a **** about native appearance of controls, and simply uses a solid outline because its designers chose so.

we can’t go that road, but what we *can* do is using special rendering for natively-rendered buttons (it appears like it has negative outline offset, but doesn’t in the CSS), and as soon as the webdev breaks the native style by giving the button’s border, background, or the outline a change, the nonnative ugly standarddesign simply has a non-offset focusring.

this fiddle shows what i mean: http://jsfiddle.net/3tPuy/

both buttons show how’d they look when focused. the upper button wouldn’t have the outline-offset property, but be rendered as if it had, and the bottom one is how the upper one would look if it got forced out of native rendering

    
  
Blocks: unprefix

    
  
Blocks: 1113202

    
    

    
  
hope remove ::-moz-focus-inner  default border styles.
Consistency with other browsers

    
    

    
  
Still not fixed in v48.  How is this possible?  I really like Firefox but umm... do the devs need some help fixing this?

Clearly the current way Firefox is still handling this is considered a bug.  The behavior is weird and everybody agrees.  Please fix the behavior of this input focus bug, because I don't feel right editing it due to accessibility reasons.

    
    

    
  
(In reply to jstnbreen from comment #45)
> do the devs need some help fixing this?

Yes, your help is welcome. Please see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction for more information. Thanks!

    
    

    
  
(In reply to Andre Klapper from comment #46)
> (In reply to jstnbreen from comment #45)
> > do the devs need some help fixing this?
> 
> Yes, your help is welcome. Please see
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Introduction for more information. Thanks!

I can try... you would think it's an easy fix.

    
    

    
  


  
Based on comments 33 and 38, here's a patch which:

- removes the -moz-focus-outer stuff from the button control frame/renderer, since I don't see it actually being used anymore (though it was used at one point for MacOS focus ring hacking).
- limits the -moz-focus-inner stuff to only affect its own extra frame, and not affect the button's normal frame or dimensions anymore.
- ensures that the -moz-focus-inner frame's border is just inside the button's border+padding, so they won't overlap (with the margin possibly tweaking it).
- changes some function names and comments to make it clearer that -moz-focus-inner only pays attention to border and margin CSS styles etc.
- modifies forms.css as appropriate to adjust for these changes.
- limits forms.css's use of -moz-focus-inner stuff to XP_WIN, since there's no reason to have it on everywhere (though perhaps it's necessary on newer Windows versions as well?).
- fixes the reftests that expected the button's normal frame to be tweaked, since that's no longer done.
- cleans up a few places in the codebase where -moz-focus-inner CSS resets were being done, but no longer have to be.

A try run doesn't seem to have any related failures, but unfortunately WinXP tries seem busted by the recent hg certificate change, so I'll have to run those later: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53cdfefdee06

        Attachment #8794987 -
        Flags: review?(dbaron)

    
    

    
  
Could you summarize what (if any) changes in the appearance of the controls (focused and non-focused) result from this patch?
Flags: needinfo?(wisniewskit)

    
    

    
  
With luck the change won't be noticeable at all. I've made an effort to have the rendering visually match as closely as I can, so that the ring is still a 1px dotted black focus ring just inside the border and padding of the button, tweaked slightly by using similar margin values (as such it should be possible to tweak those values further, in case someone from the UI team feels they need further tweaking).

The real impact of the change should be that the buttons themselves are no longer always given extra pixels of padding (whether they have a focus ring visible or not). The -moz-focus-inner focus ring will now simply be visually overlaid just inside the border and padding of the button, when it's used at all (which it won't be except for Windows XP, as other themes use a different method to render their focus ring).

If I'm being too conservative in only showing this version of the ring on XP, then it's just a minor tweak to the CSS away from being active on more OSes (or I can just omit that change, though it's wasteful to have that style active if it's never going to be used).
Flags: needinfo?(wisniewskit)

    
    

    
  
Also, note that I'll probably omit the change to dom/events/test/bug656379-1.html in the patch (second file from the top), just in case.

I can also leave out more of the CSS-resets, if we feel we're ever actually going to use this code outside of XP/Vista (since I'm guessing it can go entirely once we drop support for those OSes).

    
    

    
  
Would it show up on other platforms if '-moz-appearance: none' or one of the other styles that disables native theming (see nsRuleNode::HasAuthorSpecifiedRules and its caller) is used?
Assignee: nobody → wisniewskit
Flags: needinfo?(wisniewskit)

    
    

    
  


  
>Would it show up on other platforms if '-moz-appearance: none' or one of the other styles that disables native theming?

Ooh, good point, it wouldn't (I just confirmed that). Here's a new version of the patch without that XP_WIN bit so the styling will apply in those cases.

It also leaves a couple of the -moz-focus-inner CSS-resets intact, just to be on the safe side.

It's otherwise the same as the previous version.

        Attachment #8794987 -
        Attachment is obsolete: true

        Attachment #8794987 -
        Flags: review?(dbaron)
Flags: needinfo?(wisniewskit)

        Attachment #8795512 -
        Flags: review?(dbaron)

    
    

    
  
(Friendly review ping).

    
    

    
  
This probably would have been easier to review if the
:-moz-focus-outer removal was in a separate patch from the
:-moz-focus-inner changes, and if the test changes were in a
separate third patch (particularly since they appear in the middle
of the code changes).  It's not worth splitting it now.


In forms.css:
>   /* The sum of border and padding on block-start and block-end
>-     must be the same here, for text inputs, and for <select>.  For
>-     buttons, make sure to include the -moz-focus-inner border/padding. */
>+     must be the same here, for text inputs, and for <select>. */

Is this still true, now that the :-moz-focus-inner border/padding
don't exist anymore?

It doesn't seem like you've changed the defaults in the block
direction at all.


Prior to the patch, it looks like the amount we added in layout was
nsButterFrameRenderer::GetAddedButtonBorderAndPadding(), which was
the sum of the border and padding on the ::-moz-focus-outer and
the *margin*, border, and padding on the ::-moz-focus-inner.  These
seem to have defaulted to 3px on each side in the inline direction
and 1px in the block direction.

It looks like you've further reduced the default padding on buttons
by an additional 2px on each side in the inline direction, for a
total reduction in size of 5px on each side in the inline direction
and 1px on each side in the block direction.  Is this correct?

Or is it somehow not changing the size of buttons?


How does this change where the inner-focusring is drawn, when it's
drawn?  (Presumably it has to be closer to something, since the
buttons are now smaller.)  The code makes it look like we're using
the margin on the :-moz-focus-inner style, and putting that margin
*inside* the border (starting from the button's content box).  That
seems odd, since margin usually goes outside the border; it's not
clear why that is changed from using padding.  (Though the comment
in nsButtonFrameRenderer::PaintInnerFocusBorder and the code in
nsButtonFrameRenderer::GetButtonInnerFocusRect seem to disagree
since the code uses Inflate, but the comment seems to imply it's
inside the padding.)
Flags: needinfo?(wisniewskit)

    
    

    
  
Here is a solution:

http://www.kihlstrom.com/2016/firefox-button-padding-css-fix

This article explains the problem, and offers a solution.  The solution could be added to the default stylesheet as a fix.

    
    

    
  
No, it couldn't for reasons that should be obvious: the default stylesheet is already setting a _nonzero_ padding there, because it's needed for correct focus rect display as things stand.

    
    

    
  
>This probably would have been easier to review if...

True, sorry. I'll try to split such patches up more efficiently next time.


>Is this still true, now that the :-moz-focus-inner border/padding don't exist anymore?

Yes, I think so. Those comments seem to just be there to remind anyone changing one input type's CSS to make sure it matches the others in that regard. My patch wouldn't change the need for such consideration, just how buttons need to be considered.


>It doesn't seem like you've changed the defaults in the block direction at all.

True, I ultimately only changed what was necessary for the reftests to pass; I'll check again in case I missed something.


>It looks like you've further reduced the default padding on buttons by an additional 2px on each side in the inline direction, for a total reduction in size of 5px on each side in the inline direction and 1px on each side in the block direction.  Is this correct?

Yes, that's correct.


>How does this change where the inner-focusring is drawn, when it's drawn?

It's still drawn just inside the border and padding of the button's content-box, but now it overlaps it along the edges instead of adjusting the size of the content-box to accommodate for itself. Visually that means it will shrink toward the content compared to the old version.


>That seems odd, since margin usually goes outside the border

Indeed it does. I don't mind using Deflate with negative values instead (the purpose was of course just to nudge the focus ring a bit away from the content-box so it doesn't overlap as obviously, and better-match what the old appearance looked like).
Flags: needinfo?(wisniewskit)

    
    

    
  
(In reply to Thomas Wisniewski from comment #58)
> >That seems odd, since margin usually goes outside the border
> 
> Indeed it does. I don't mind using Deflate with negative values instead (the
> purpose was of course just to nudge the focus ring a bit away from the
> content-box so it doesn't overlap as obviously, and better-match what the
> old appearance looked like).

I guess what I was suggesting is using padding instead of margin there (and specifying the styles as padding rather than margin).  At least if I'm correctly understanding what it's doing...

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #59)
> I guess what I was suggesting is using padding instead of margin there (and
> specifying the styles as padding rather than margin).  At least if I'm
> correctly understanding what it's doing...

Sure, I can change it to Inflate by the padding instead of using the margin, that's fine (again, the only reason to even have padding/margin there is to nudge the new focus ring outward slightly into the padding, so it's not overlapping the edges of the content as tightly).

    
    

    
  
So this is making the default size of buttons with -moz-appearance:none and on certain (which?) windows versions smaller by 10px in the inline dimension and 2px in the block dimension?
Flags: needinfo?(wisniewskit)

    
    

    
  
Starting to seem very complicated.  Is there anyway to see how Safari or other browsers have done this?  Please excuse me if that's stating the obvious.

    
    

    
  
>So this is making the default size of buttons with -moz-appearance:none and on certain (which?) windows versions smaller by 10px in the inline dimension and 2px in the block dimension?

It might be easier to view this as two changes that affect the size. Firstly the 2px of space that's set aside for the actual focus-ring, and then the other 8px of inline padding that we could keep (if we'd like to).

The pre-patch code actually *always* sets aside the 2px necessary for the focus-ring, regardless of whether the button is focused or the theme will ever actually paint it (that's the code I remove from nsHTMLButtonControlFrame.cpp). As such, *all* buttons will shrink by that 2px. It's only when it comes time to paint the outline in that space that the theme check is made (and I believe all Windows versions apply, given that the code in nsNativeThemeWin::ThemeDrawsFocusForWidget() always returns false, so unless I'm wrong nsDisplayButtonForeground::Paint() would always try to paint it for Windows).

The other 8px of inline padding was removed in the CSS as part of an effort to get the reftests passing again. It also affects all buttons regardless of theme. I can try re-working the patch to retain that 8px if we'd like to keep it (limiting the shrinkage to just the 2px).
Flags: needinfo?(wisniewskit)

    
    

    
  
So when I asked in comment 49 how this would change the appearance of the controls, I'd have hoped you'd have told me that they'd be a different size...

I find it somewhat curious that shrinking the controls further made it easier to get the reftests to pass.  Why was that?

In general, though, I think we should be making decisions about what behavior to expose to the Web based on what we think is best for the Web, not on what makes it easiest to get the tests to pass.  (Although sometimes being able to move on to other things more quickly is best for the Web, so it's not unacceptable to consider.)

So how do the changes in the size of the controls compare to what other browsers do?
Flags: needinfo?(wisniewskit)

    
    

    
  
>I find it somewhat curious that shrinking the controls further made it easier to get the reftests to pass.  Why was that?

To be honest, I no longer remember why. It seems it would be best for me to give the patch a do-over, trying to not remove the 8px padding, but just the 2px for the outline itself. Then I'll at least know for sure.
Flags: needinfo?(wisniewskit)

    
    

    
  


  
I'm adding a testcase with some embedded results comparing Firefox to Chrome and Safari on macOS (only OS I have around right now).

For a <button style="padding:0">, Firefox has 2 extra pixels vertically and horizontally, which this bug is trying to remove (to help authors achieve more consistent and predictable styling).

If the result of a patch is that buttons get 2px smaller on both directions, that should be alright. Not sure about where a 8px reduction on the horizontal axis would come from.

(Additionally, the default, platform-specific style for the button on macOS is 16px wider in Firefox than in Chrome and Safari, but that’s unrelated to this bug.)

    
    

    
  


  
Alright, I re-worked the patch to only remove the 2px. Indeed I can't figure out why I removed the 8px last time; I must have just been over (or under) thinking things.

The meat of the patch hasn't actually changed. I just don't tinker with the CSS padding as much, and I went over the tests again to see which should just be removed, and which just needed minor tweaks because -moz-focus-inner no longer affects the button's width.

I've also split the patch up, just in case it helps simplify subsequent review. Here's part one, which just removes the moz-focus-outer bits.

        Attachment #8795512 -
        Attachment is obsolete: true

        Attachment #8795512 -
        Flags: review?(dbaron)

        Attachment #8803455 -
        Flags: review?(dbaron)

    
    

    
  


  
Here's part two, the meat of the patch that actually gets rid of the extra padding for the inner-ring.

        Attachment #8803460 -
        Flags: review?(dbaron)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        140562-3-fix_up_reftests.diff (obsolete)
        — 
          — Splinter Review
      

    


  
And here are the reftest fixes to account for the fact that -moz-focus-inner no longer affects the button's size.

        Attachment #8803462 -
        Flags: review?(dbaron)

    
    

    
  
Comment on attachment 8803455 [details] [diff] [review]
140562-1-remove_unused_moz_focus_outer_code_for_buttons.diff

r=dbaron.

Sorry for taking so long to get back to these.

        Attachment #8803455 -
        Flags: review?(dbaron) → review+

    
    

    
  
Comment on attachment 8803460 [details] [diff] [review]
140562-2-remove_2px_padding_on_buttons_for_moz-focus-inner_ring.diff

>+nsButtonFrameRenderer::GetButtonInnerFocusRect(const nsRect& aRect, nsRect& focusRect)

The second parameter should be aResult (like it is in the header) rather
than focusRect.


>   ReflowInput contentsReflowInput(aPresContext,
>-                                        adjustedButtonReflowInput,
>+                                        aButtonReflowInput,
>                                         aFirstKid, availSize);

While you're here, please fix the indentation to line up with aPresContext.


>   nscoord extraSpace =
>-    buttonContentBox.BSize(wm) - focusPadding.BStartEnd(wm) -
>+    buttonContentBox.BSize(wm) -
>     contentsDesiredSize.BSize(wm);

Please fit this on 2 lines rather than 3.


>-    if (focusRingWidth != 1) {
>-      // If the focus ring width is different from the default, fix buttons
>-      // with rings.
>-      sheetText.AppendPrintf(
>-          "button::-moz-focus-inner, input[type=\"reset\"]::-moz-focus-inner, "
>-          "input[type=\"button\"]::-moz-focus-inner, "
>-          "input[type=\"submit\"]::-moz-focus-inner { "
>-          "padding: 1px 2px 1px 2px; "
>-          "border: %dpx %s transparent !important; }\n",
>-          focusRingWidth,
>-          focusRingStyle == 0 ? "solid" : "dotted");
>-
>-      sheetText.AppendLiteral(
>-          "button:focus::-moz-focus-inner, "
>-          "input[type=\"reset\"]:focus::-moz-focus-inner, "
>-          "input[type=\"button\"]:focus::-moz-focus-inner, "
>-          "input[type=\"submit\"]:focus::-moz-focus-inner { "
>-          "border-color: ButtonText !important; }\n");
>-    }

It's not entirely clear to me that this should be removed... although it's
also not clear to me that this code actually works in any useful way either.

(The code is obviously somewhat buggy in that it ignores the focus ring
style when the width is its default.)

I'd be OK with removing the padding line only and leaving the rest, although
I'm also OK with removing it.


I'm still vaguely concerned about the 2px reduction in size in each
dimension... seems like something that might help compat on some sites
and harm it on others... but we may as well try.

r=dbaron

        Attachment #8803460 -
        Flags: review?(dbaron) → review+

    
    

    
  
Thanks for the reviews, dbaron! I'll address the nits ASAP. You could be right about that block of code, I'll try just removing the padding for now.

Also, if there are compat issues, I suspect I'll hear about them pretty soon (given that I'm on the webcompat team now). So I'll be around to help with any fallout :)

    
    

    
  
Alright, I've managed to update the patches, and I think they may be good to go... but I'm concerned about one try-failure I'm seeing on Android, for widget/cocoa/crashtests/449111-1.html: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e3a88bd3b1e10468d7d336599b677f2e6f78786&selectedJob=32127184

I can't tell from the log which assertion is being triggered. Does it just mean that the crashtest crashed? The build artifact doesn't seem to be there, so how I can I verify this, short of building my own and testing it? I'm also unsure how my code-changes could cause this test to start failing, let alone just on Android:

><html>
><head></head>
><body><div style="display: -moz-box; word-spacing: 549755813889px;"><button>T </button></div></body>
></html>

Any tips?
Flags: needinfo?(dbaron)

    
    

    
  
Thomas, the failure is actually there on all platforms.  You're just not seeing the other ones turn orange because of bug 1321127.

Looking at the linux64 debug crashtest log at https://treeherder.mozilla.org/logviewer.html#?job_id=32132771&repo=try#L117893 the assert is:

 [task 2016-12-01T05:38:39.787808Z] 05:38:39 INFO - [1012] ###!!! ASSERTION: Should have real computed inline-size by now: 'aReflowInput.ComputedISize() != nscoord(1 << 30)', file /home/worker/workspace/build/src/layout/forms/nsHTMLButtonControlFrame.cpp, line 188 

and there's a stack and whatnot.

    
    

    
  
Thanks bz! I'm on it...
Flags: needinfo?(dbaron)

    
    

    
  
So it's not really that my patches are causing a failure; the assertion just thinks that no inline-size has been computed yet, though it has (it just turns out that the button's ComputedISize == NS_INTRINSICSIZE):

>  NS_PRECONDITION(aReflowInput.ComputedISize() != NS_INTRINSICSIZE,
                  "Should have real computed inline-size by now");

This is true even on a build without my patchset, if you just disable the focuspadding in forms.css (by setting -moz-focus-inner's border and padding to zero).

However, I'm not sure what a "proper" solution would be;
1. just remove the assertion
2. add some sort of flag to indicating whether a value was indeed computed (even if it happens to be equal to NS_INTRINSICSIZE)
3. ever-so-slightly fudge the value away from NS_INTRINSICSIZE so the assertion isn't tripped

dbaron, bz, what do you guys think?
Flags: needinfo?(dbaron)
Flags: needinfo?(bzbarsky)

    
    

    
  
Given what the test (widget/cocoa/crashtests/449111-1.html) is testing, I think you should just remove the assertion.

I think that assertion should be seen as a transitional thing from the reflow branch (bug 300030) when we were converting things from using NS_UNCONSTRAINEDSIZE / NS_INTRINSICSIZE to mean that Reflow had to figure it out, to doing that in a separate intrinsic width computation pass before reflow.
Flags: needinfo?(dbaron)

    
    

    
  
Agreed on removing the assertion.
Flags: needinfo?(bzbarsky)

    
    

    
  


  
Alright, thanks to the both of you! Final patches are incoming, with that assertion removed and review comments addressed (the first patch required no changes).

The other patches required minor tweaks, as the CSS and tests had changed slightly since the patches were written, so I don't believe it warrants a re-review or re-try. But for posterity, the changes included a couple of changes one would expect to forms.css and these test that now no longer have to take focuspadding into account:
- layout/reftests/css-grid/grid-item-intrinsic-size-normal-001.html
- layout/reftests/forms/button/width-auto-size-em-ltr-ref.html

The patches are applying cleanly to inbound on my end, and the try-run was fine aside from the removal of that assertion, so I'm going to carry over r+ and request check-in for all three patches.

        Attachment #8803460 -
        Attachment is obsolete: true

    
    

    
  


  
And here is the new reftest patch.

        Attachment #8803462 -
        Attachment is obsolete: true

    
    

    
  
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f0eba2dcbe
Part 1: Remove unused moz-focus-outer code for buttons. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/f86ce2e2f6a5
Part 2: Remove the 2px extra padding on buttons for a prospective -moz-focus-inner ring, and just size that ring the same as the content frame (inflated by its CSS padding). r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/4855aa893e36
Part 3: Fix up reftests to account for the fact that -moz-focus-inner no longer affects button size. r=dbaron
Keywords: checkin-needed

    
    

    
  
I'm updating this bug to dev-doc-complete - we mention moz-focus-inner in a couple of places as being the pseudo-element to use to override the default accessibility focus styling (e.g. in the <button> page), but we don't mention this bug anywhere, so it looks like the docs are correct as they stand.
Keywords: dev-doc-needed → dev-doc-complete

    
  
Depends on: 1349646

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: