Last Comment Bug 140562 - Replace -moz-focus-inner from form elements with standard outline
: Replace -moz-focus-inner from form elements with standard outline
Status: RESOLVED FIXED
workaround in comment 14
: dev-doc-complete, testcase
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
P1 normal with 28 votes (vote)
: Future
Assigned To: Thomas Wisniewski
:
: Jet Villegas (:jet)
Mentors:
: 183335 208896 355849 430439 435697 448893 489284 573978 594980 611969 635698 642149 718350 812907 (view as bug list)
Depends on:
Blocks: unprefix 1113202 418521 654225
  Show dependency treegraph
 
Reported: 2002-04-27 08:06 PDT by Jens Schiffler
Modified: 2017-02-20 07:45 PST (History)
43 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
-


Attachments
testcase (357 bytes, text/html)
2002-04-27 10:49 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
testcase from bug 208896 (2.66 KB, text/html)
2005-06-15 07:20 PDT, Yuh-Ruey Chen
no flags Details
testcase 3 (328 bytes, text/html)
2010-06-23 17:48 PDT, j.j.
no flags Details
left: wrong firefox focus-ring, right: native focus-ring (2.25 KB, image/png)
2012-01-16 10:12 PST, flying sheep
no flags Details
Patch v1 (1.37 KB, patch)
2012-09-20 16:25 PDT, Josh Tumath
bzbarsky: feedback+
Details | Diff | Splinter Review
140562-simplify-hacks-for-moz-focus-inner-and-outer-in-buttons.diff (61.65 KB, patch)
2016-09-26 12:59 PDT, Thomas Wisniewski
no flags Details | Diff | Splinter Review
140562-simplify-hacks-for-moz-focus-inner-and-outer-in-buttons.diff (59.86 KB, patch)
2016-09-27 14:58 PDT, Thomas Wisniewski
no flags Details | Diff | Splinter Review
testcase: default button padding (1.70 KB, text/html)
2016-10-21 02:58 PDT, Florens Verschelde
no flags Details
140562-1-remove_unused_moz_focus_outer_code_for_buttons.diff (16.01 KB, patch)
2016-10-21 11:04 PDT, Thomas Wisniewski
dbaron: review+
Details | Diff | Splinter Review
140562-2-remove_2px_padding_on_buttons_for_moz-focus-inner_ring.diff (20.57 KB, patch)
2016-10-21 11:06 PDT, Thomas Wisniewski
dbaron: review+
Details | Diff | Splinter Review
140562-3-fix_up_reftests.diff (32.54 KB, patch)
2016-10-21 11:07 PDT, Thomas Wisniewski
dbaron: review+
Details | Diff | Splinter Review
140562-2-remove_2px_padding_on_buttons_for_moz-focus-inner_ring.diff (21.99 KB, patch)
2016-12-01 22:21 PST, Thomas Wisniewski
no flags Details | Diff | Splinter Review
140562-3-fix_up_reftests.diff (37.59 KB, patch)
2016-12-01 22:22 PST, Thomas Wisniewski
no flags Details | Diff | Splinter Review

Description User image Jens Schiffler 2002-04-27 08:06:51 PDT
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>
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2002-04-27 10:49:28 PDT
Created attachment 81300 [details]
testcase
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2002-04-27 10:51:17 PDT
Confirming bug -- the button still has padding...
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2002-12-03 19:47:05 PST
*** Bug 183335 has been marked as a duplicate of this bug. ***
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2002-12-03 19:47:44 PST
the only way to solve this is to fix our focus rect stuff...
Comment 5 User image Carine Held 2003-01-07 11:17:27 PST
Conirmed in 1/03/03 Trunk build, Linux. Testcase keyword added.
Comment 6 User image Pascal Chevrel:pascalc 2003-06-10 11:09:53 PDT
*** Bug 208896 has been marked as a duplicate of this bug. ***
Comment 7 User image Yuh-Ruey Chen 2005-06-15 07:20:39 PDT
Created attachment 186323 [details]
testcase from bug 208896

testcase with <a> element within <button> element
Comment 8 User image nrlz 2006-11-02 21:40:14 PST
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>
Comment 9 User image Marat Tanalin | tanalin.com 2007-09-24 08:24:02 PDT
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).
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2008-05-03 12:30:00 PDT
*** Bug 355849 has been marked as a duplicate of this bug. ***
Comment 11 User image Boris Zbarsky [:bz] (still a bit busy) 2008-05-03 12:31:43 PDT
Note that we use the horizontal padding to do the "pressed" look as well.  See bug 179230.
Comment 12 User image Paul LeBeau 2008-05-06 07:36:24 PDT
There is another test case in bug 355849.
Comment 13 User image j.j. 2010-06-23 16:05:12 PDT
*** Bug 573978 has been marked as a duplicate of this bug. ***
Comment 14 User image j.j. 2010-06-23 17:48:16 PDT
Created attachment 453599 [details]
testcase 3

workaround (from bug 573978)) is
::-moz-focus-inner { padding:0 }
::-moz-focus-inner { padding:0; border:0 }  /* ! removes focus line */
Comment 15 User image flying sheep 2010-10-12 18:11:10 PDT
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.
Comment 16 User image flying sheep 2010-10-12 18:34:09 PDT
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!
Comment 17 User image Emil Ivanov 2010-11-01 10:54:29 PDT
*** Bug 448893 has been marked as a duplicate of this bug. ***
Comment 18 User image Nochum Sossonko [:Natch] 2010-11-01 11:18:30 PDT
*** Bug 435697 has been marked as a duplicate of this bug. ***
Comment 19 User image Emil Ivanov 2010-11-01 11:40:14 PDT
*** Bug 594980 has been marked as a duplicate of this bug. ***
Comment 20 User image Emil Ivanov 2011-03-28 04:44:41 PDT
*** Bug 635698 has been marked as a duplicate of this bug. ***
Comment 21 User image Emil Ivanov 2011-03-28 05:00:29 PDT
*** Bug 642149 has been marked as a duplicate of this bug. ***
Comment 22 User image Emil Ivanov 2011-03-28 05:08:58 PDT
*** Bug 489284 has been marked as a duplicate of this bug. ***
Comment 23 User image Emil Ivanov 2011-04-06 09:13:59 PDT
*** Bug 611969 has been marked as a duplicate of this bug. ***
Comment 24 User image Emil Ivanov 2011-04-06 09:21:08 PDT
*** Bug 430439 has been marked as a duplicate of this bug. ***
Comment 25 User image Josh Tumath 2011-04-07 10:54:23 PDT
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?
Comment 26 User image Mounir Lamouri (:mounir) 2011-04-12 14:05:24 PDT
(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/
Comment 27 User image Nochum Sossonko [:Natch] 2011-04-12 14:14:42 PDT
Another great resource is MDN (mozilla developer network). Information on building Firefox is available here: https://developer.mozilla.org/En/Developer_Guide/Build_Instructions
Comment 28 User image Florens Verschelde 2011-10-26 07:52:48 PDT
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;
}
Comment 29 User image Boris Zbarsky [:bz] (still a bit busy) 2012-01-15 17:35:26 PST
*** Bug 718350 has been marked as a duplicate of this bug. ***
Comment 30 User image Boris Zbarsky [:bz] (still a bit busy) 2012-01-15 17:41:37 PST
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.
Comment 31 User image flying sheep 2012-01-16 10:12:25 PST
Created attachment 588924 [details]
left: wrong firefox focus-ring, right: native focus-ring

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.
Comment 32 User image Alex Limi (:limi) — Firefox UX Team 2012-02-03 16:16:46 PST
I think this is a bit too low-level for the UX team to have any specific opinion about. Sorry!
Comment 33 User image Mounir Lamouri (:mounir) 2012-04-18 10:46:55 PDT
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
Comment 34 User image Josh Tumath 2012-09-20 16:25:35 PDT
Created attachment 663203 [details] [diff] [review]
Patch v1

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.
Comment 35 User image Mounir Lamouri (:mounir) 2012-09-21 03:23:30 PDT
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.
Comment 36 User image Boris Zbarsky [:bz] (still a bit busy) 2012-09-21 07:21:24 PDT
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.
Comment 37 User image Josh Tumath 2012-09-21 07:41:19 PDT
(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.
Comment 38 User image Boris Zbarsky [:bz] (still a bit busy) 2012-09-21 07:55:30 PDT
> 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.
Comment 39 User image Josh Tumath 2012-09-21 08:00:36 PDT
(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.
Comment 40 User image flying sheep 2012-09-23 11:08:46 PDT
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.
Comment 41 User image Boris Zbarsky [:bz] (still a bit busy) 2012-09-23 13:39:56 PDT
> 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.
Comment 42 User image flying sheep 2012-09-28 06:04:50 PDT
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
Comment 43 User image Boris Zbarsky [:bz] (still a bit busy) 2012-11-20 07:56:33 PST
*** Bug 812907 has been marked as a duplicate of this bug. ***
Comment 44 User image 709922234 2016-07-17 21:59:01 PDT
hope remove ::-moz-focus-inner  default border styles.
Consistency with other browsers
Comment 45 User image jstnbreen 2016-08-04 09:30:19 PDT
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.
Comment 46 User image Andre Klapper 2016-08-04 09:34:11 PDT
(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!
Comment 47 User image jstnbreen 2016-08-04 09:49:15 PDT
(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.
Comment 48 User image Thomas Wisniewski 2016-09-26 12:59:03 PDT
Created attachment 8794987 [details] [diff] [review]
140562-simplify-hacks-for-moz-focus-inner-and-outer-in-buttons.diff

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
Comment 49 User image David Baron :dbaron: ⌚️UTC-8 2016-09-27 12:39:38 PDT
Could you summarize what (if any) changes in the appearance of the controls (focused and non-focused) result from this patch?
Comment 50 User image Thomas Wisniewski 2016-09-27 12:58:23 PDT
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).
Comment 51 User image Thomas Wisniewski 2016-09-27 13:06:25 PDT
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).
Comment 52 User image David Baron :dbaron: ⌚️UTC-8 2016-09-27 13:27:25 PDT
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?
Comment 53 User image Thomas Wisniewski 2016-09-27 14:58:16 PDT
Created attachment 8795512 [details] [diff] [review]
140562-simplify-hacks-for-moz-focus-inner-and-outer-in-buttons.diff

>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.
Comment 54 User image Thomas Wisniewski 2016-10-17 06:10:59 PDT
(Friendly review ping).
Comment 55 User image David Baron :dbaron: ⌚️UTC-8 2016-10-19 17:17:54 PDT
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.)
Comment 56 User image jstnbreen 2016-10-20 07:49:55 PDT
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.
Comment 57 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-20 07:51:27 PDT
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.
Comment 58 User image Thomas Wisniewski 2016-10-20 09:41:25 PDT
>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).
Comment 59 User image David Baron :dbaron: ⌚️UTC-8 2016-10-20 11:01:37 PDT
(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...
Comment 60 User image Thomas Wisniewski 2016-10-20 11:08:22 PDT
(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).
Comment 61 User image David Baron :dbaron: ⌚️UTC-8 2016-10-20 14:37:00 PDT
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?
Comment 62 User image jstnbreen 2016-10-20 14:44:21 PDT
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.
Comment 63 User image Thomas Wisniewski 2016-10-20 17:26:08 PDT
>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).
Comment 64 User image David Baron :dbaron: ⌚️UTC-8 2016-10-20 17:35:17 PDT
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?
Comment 65 User image Thomas Wisniewski 2016-10-20 19:37:42 PDT
>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.
Comment 66 User image Florens Verschelde 2016-10-21 02:58:49 PDT
Created attachment 8803284 [details]
testcase: default button padding

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.)
Comment 67 User image Thomas Wisniewski 2016-10-21 11:04:43 PDT
Created attachment 8803455 [details] [diff] [review]
140562-1-remove_unused_moz_focus_outer_code_for_buttons.diff

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.
Comment 68 User image Thomas Wisniewski 2016-10-21 11:06:45 PDT
Created attachment 8803460 [details] [diff] [review]
140562-2-remove_2px_padding_on_buttons_for_moz-focus-inner_ring.diff

Here's part two, the meat of the patch that actually gets rid of the extra padding for the inner-ring.
Comment 69 User image Thomas Wisniewski 2016-10-21 11:07:37 PDT
Created attachment 8803462 [details] [diff] [review]
140562-3-fix_up_reftests.diff

And here are the reftest fixes to account for the fact that -moz-focus-inner no longer affects the button's size.
Comment 70 User image David Baron :dbaron: ⌚️UTC-8 2016-11-29 14:16:20 PST
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.
Comment 71 User image David Baron :dbaron: ⌚️UTC-8 2016-11-29 14:49:11 PST
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
Comment 72 User image Thomas Wisniewski 2016-11-29 15:26:37 PST
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 :)
Comment 73 User image Thomas Wisniewski 2016-12-01 09:01:09 PST
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?
Comment 74 User image Boris Zbarsky [:bz] (still a bit busy) 2016-12-01 09:29:56 PST
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.
Comment 75 User image Thomas Wisniewski 2016-12-01 09:35:12 PST
Thanks bz! I'm on it...
Comment 76 User image Thomas Wisniewski 2016-12-01 14:55:10 PST
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?
Comment 77 User image David Baron :dbaron: ⌚️UTC-8 2016-12-01 16:18:18 PST
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.
Comment 78 User image Boris Zbarsky [:bz] (still a bit busy) 2016-12-01 21:30:54 PST
Agreed on removing the assertion.
Comment 79 User image Thomas Wisniewski 2016-12-01 22:21:43 PST
Created attachment 8816382 [details] [diff] [review]
140562-2-remove_2px_padding_on_buttons_for_moz-focus-inner_ring.diff

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.
Comment 80 User image Thomas Wisniewski 2016-12-01 22:22:11 PST
Created attachment 8816383 [details] [diff] [review]
140562-3-fix_up_reftests.diff

And here is the new reftest patch.
Comment 81 User image Thomas Wisniewski 2016-12-01 22:25:55 PST
Checkin requested for the three reviewed patches:
1. attachment #8803455 [details] [diff] [review]
2. attachment #8816382 [details] [diff] [review]
3. attachment #8816383 [details] [diff] [review]
Comment 82 User image Pulsebot 2016-12-02 00:20:34 PST
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
Comment 84 User image Chris Mills (Mozilla, MDN editor) [:cmills] 2017-02-20 07:45:20 PST
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.

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