Closed Bug 875275 Opened 11 years ago Closed 11 years ago

Implement <input type="color">: layout part

Categories

(Core :: Layout: Form Controls, defect)

Product:
Core
Component:
Layout: Form Controls
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla27

People

(Reporter: arnaud.bienner, Assigned: arnaud.bienner)

References

Details

Attachments

(6 files, 14 obsolete files)

Color input: layout part (9th) [rebased & s/1-ref/ref-1/]
44.48 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
part 4: tweak reftests to handle being button-based
13.55 KB, patch
arnaud.bienner
: review+
Details | Diff | Splinter Review
part 3 v2: inherit from nsHTMLButtonControlFrame [feedback=arnaud.bienner]
7.75 KB, patch
jwatt
: review+
dholbert
: feedback+
Details | Diff | Splinter Review
part 2 v2: simplify forms.css
1.76 KB, patch
arnaud.bienner
: review+
Details | Diff | Splinter Review
part 5: Fix reftest failure on windows by specifying "padding" in testcases
3.25 KB, patch
arnaud.bienner
: review+
Details | Diff | Splinter Review
part 6: annotate tests as failing on B2G due to bug 928877
1.02 KB, patch
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:21.0) Gecko/20100101 Firefox/21.0 (Beta/Release)
Build ID: 20130511120803
Blocks: 547004
OS: Windows XP → All
Hardware: x86 → All
To track the implementation of the layout part of the <input type="color" />
Assignee: nobody → arnaud.bienner
Depends on: 875274
Blocks: 875747
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Color input: layout part (3rd) (obsolete) — — Splinter Review 
Mounir, David, could you please have a look to this patch?
Everything is working as expected, except the pseudo element: I tried to create a -moz-color-swatch pseudo element for the inner div (which represents the selected color). It partly works: the element is stylable, but then the "mColorContent->SetAttr(bgcolor, color)" doesn't work anymore (mColorContent object is changed but the element doesn't seem to be visible, like if it was removed from the DOM tree after being associated with the pseudo element).
So I commented this part in my patch.
Attachment #766287 - Flags: feedback?(mounir)
Attachment #766287 - Flags: feedback?(dbaron)
Is there a description somewhere of what sort of UI you expect <input type="color"> to have?

Why is this patch adding style mappings for a bgcolor attribute to all <div> elements?  You shouldn't add a new Web-facing feature just because you have an internal need to set the color of a div.

The GetColor() method returning nsString seems like it's going to lead to more nsString constructor calls than needed.  It might be better to use an out-param since it's wrapping something that involves an out-param.
The UI is going to be some kind of button with a preview of the color inside it, like you have on native UIs. Like Chrome does.
When you click on the button, it open a color picker like the file picker does but that part is handled in a different part (the content code will do it).

I agree regarding the bgcolor attribute and I mentioned this to Arnaud this week-end. Do you have any suggestion regarding how to do that instead?
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #3) 
> Why is this patch adding style mappings for a bgcolor attribute to all <div>
> elements?  You shouldn't add a new Web-facing feature just because you have
> an internal need to set the color of a div.

Indeed, sorry: I didn't realized the impacts, and that it breaks the current behavior.
Do you have any idea how I can achieve the same effect?
I guess changing the element's style in some way might do the trick, but I wasn't able to find any example like this in the codebase.
Any idea is more than welcome :)
 
> The GetColor() method returning nsString seems like it's going to lead to
> more nsString constructor calls than needed.  It might be better to use an
> out-param since it's wrapping something that involves an out-param.

I don't really like out param as it makes the code hard to read, and can lead to unseen errors (e.g. "A a; getValue(a);" will not raised any warnings if 'a' is unused then, while "A a = getValue();" will do so); while in the meantime compilers easily optimize this kind of things (e.g. I know gcc performs RVO even with -O0).
Comment on attachment 766287 [details] [diff] [review]
Color input: layout part (3rd)

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

Some minor comments:
- You should revert the changes to layout/forms/nsHTMLButtonControlFrame.cpp;
- You are adding a lot of whitespaces.

Also, I think we should clean up forms.css to keep to first disable all properties set by the |input| rule that applies to <input type='color'>. We should probably set some properties like width and height. I think the height should be relative to em (like progress). I'm not sure about the width. What others do?

The main problem in that patch is setting the color of the swatch and make the pseudo element usable. I unfortunately can't help with that. Hopefully David will be able to give some hints.

::: content/html/content/src/HTMLInputElement.cpp
@@ +2104,5 @@
> +      // and updated
> +      if (mType == NS_FORM_INPUT_COLOR) {
> +        return nsGenericHTMLFormElement::SetAttr(kNameSpaceID_None,
> +                                                 nsGkAtoms::value, aValue,
> +                                                 true);

Wouldn't that be better to invalidate the frame instead?

::: layout/forms/nsColorControlFrame.cpp
@@ +54,5 @@
> +// The frame will be generated by the frame constructor.
> +nsresult
> +nsColorControlFrame::CreateAnonymousContent(nsTArray<ContentInfo>& aElements)
> +{
> +  nsString color = GetColor();

Do that call when you need it.

@@ +87,5 @@
> +      return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +
> +  nsresult rv = mColorContent->SetAttr(kNameSpaceID_None, nsGkAtoms::bgcolor,
> +                         color, true);

return mColorContent->SetAttr(...);

@@ +110,5 @@
> +{
> +}
> +
> +nsresult nsColorControlFrame::SetFormProperty(nsIAtom* aName, const nsAString& aValue)
> +{

I'm trying to get ride of SetFormProperty and GetFormProperty. When do you need that?

@@ +150,5 @@
> +                           color, true);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }
> +  rv = nsBlockFrame::AttributeChanged(aNameSpaceID, aAttribute, aModType);
> +  return rv;

return nsBlockFrame::AttributeChanged(...);

::: layout/forms/nsColorControlFrame.h
@@ +16,5 @@
> +#endif
> +
> +// Class which implements the input type=color
> +
> +class nsColorControlFrame : public nsBlockFrame,

Make the class MOZ_FINAL.

@@ +22,5 @@
> +                            public nsIAnonymousContentCreator
> +{
> +public:
> +  nsColorControlFrame(nsStyleContext* aContext);
> +  virtual ~nsColorControlFrame() {};

... which would allow you to remove this safely.

@@ +24,5 @@
> +public:
> +  nsColorControlFrame(nsStyleContext* aContext);
> +  virtual ~nsColorControlFrame() {};
> +
> +  virtual void DestroyFrom(nsIFrame* aDestructRoot) MOZ_OVERRIDE;

And will remove the need of marking this method |virtual|.

@@ +30,5 @@
> +  NS_DECL_QUERYFRAME_TARGET(nsColorControlFrame)
> +  NS_DECL_FRAMEARENA_HELPERS
> +  NS_DECL_QUERYFRAME
> +
> +  virtual nsIAtom* GetType() const MOZ_OVERRIDE;

and this one ... and the others below ;)

@@ +67,5 @@
> +  // Returns the color element value as a string, which is supposed to be a hex triplet
> +  nsString GetColor() const;
> +
> +private:
> +  nsCOMPtr<nsIContent> mColorContent;

Why protected and private? Protected doesn't sound terribly useful here or am I missing something?

@@ +72,5 @@
> +};
> +
> +
> +#endif
> +

Remove the empty line and change #endif to #enidf // nsColorControlFrame_h___

::: layout/style/nsCSSPseudoElementList.h
@@ +57,5 @@
>  CSS_PSEUDO_ELEMENT(mozRangeProgress, ":-moz-range-progress", 0)
>  CSS_PSEUDO_ELEMENT(mozRangeThumb, ":-moz-range-thumb", 0)
>  CSS_PSEUDO_ELEMENT(mozMeterBar, ":-moz-meter-bar", 0)
>  CSS_PSEUDO_ELEMENT(mozPlaceholder, ":-moz-placeholder", 0)
> +CSS_PSEUDO_ELEMENT(mozColorSwatch, ":-moz-color-swatch", 0)

I'm not a big fan of the name but if webkit has :-webkit-color-swatch at least we can claim consistency.
Attachment #766287 - Flags: feedback?(mounir)
Comment on attachment 766287 [details] [diff] [review]
Color input: layout part (3rd)

>+  // TODO: Associate ::-moz-color-swatch pseudo-element to the anonymous child.
>+  // Currently it partly works: element is stylable but mColorContent->SetAttr
>+  // doesn't work anymore then.
>+  /*nsCSSPseudoElements::Type pseudoType = nsCSSPseudoElements::ePseudo_mozColorSwatch;
>+  nsRefPtr<nsStyleContext> newStyleContext = PresContext()->StyleSet()->
>+    ResolvePseudoElementStyle(mContent->AsElement(), pseudoType,
>+                              StyleContext());
>+  if (!aElements.AppendElement(ContentInfo(mColorContent, newStyleContext))) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }*/

So this bit seems like it should work; it's what nsProgressFrame uses (bug 654989, bug 654990).


As far as replacing the SetAttr:  perhaps try SetAttr() but setting the style attribute to background-color: #rrggbb.  That's ugly, but it seems like it ought to work.  (Text inputs used to do something like that, but we fixed them not to in bug 243588.)  I can't really think of a better idea right now.
Attachment #766287 - Flags: feedback?(dbaron) → feedback-
Arnaud, I got an idea regarding how to push this part of the feature without making disabling <input type='color'> not working: I think you should not change forms.css with the layout patch. Basically, as long as we don't touch forms.css, we can safely keep everything behind the pref so we could land that patch and add the backends when they are ready. Then, when we have all the backends and the feature will be ready to ship, we could add the forms.css changes and remove the pref (given that, at that point, the pref would be useless). Do you think that could work? In other words, would the feature work if there is no stylesheet for the input type? Being ugly is okay as long as it is usable.
Flags: needinfo?(arnaud.bienner)
Attached patch Color input: layout part (4th) (obsolete) — — Splinter Review 
I've updated my patch to revert changes on HTML div and use setAttr(style) instead in the color control frame (+ some cleaning).
It's working as I expect.
However, the pseudo-element still doesn't work, and I'm really stuck here with this: the part I've commented is indeed similar to what have been done in nsProgressFrame.cpp (actually I used this, nsMeterFrame and nsRangeFrame as examples), and let me customize the pseudo element through CSS, as for progress frame. But the setAttr doesn't work then. As the examples I've looked at don't need to use setAttr, I can't figure out what I'm missing here.
If we have no idea why it's not working, I would suggest to leave the pseudo element support for later, so we aren't blocked because of this.
Attachment #766287 - Attachment is obsolete: true
Attachment #773223 - Flags: feedback?(dbaron)
Flags: needinfo?(arnaud.bienner)
(In reply to Arnaud from comment #9)
> If we have no idea why it's not working, I would suggest to leave the pseudo
> element support for later, so we aren't blocked because of this.

I think it would be great to have a basic layout quickly to make the backends implementation easier so I would definitely be in favour in implementing the pseudo-element support in another bug. This said, we will need this in order to ship.
Comment on attachment 773223 [details] [diff] [review]
Color input: layout part (4th)

Oh, right, it doesn't work because nsHTMLCSSStyleSheet::RulesMatching(AnonBoxRuleProcessorData* aData)
doesn't do anything.  And it would actually be sort of hard to fix in the current architecture since it doesn't have access to the pseudo-element, but only to the element.
Comment on attachment 773223 [details] [diff] [review]
Color input: layout part (4th)

So I think there are 2 options:

(1) something much like what nsTextControlFrame does with its .anonymous-div (but use a diferent class name, maybe .color-swatch):  don't use a pseudo-element, just use a class, have a selector in forms.css that refers to it as a child of input, and depend on the fact that author selectors won't match native-anonymous content.  It's not quite perfect since the presence of that selector is detectable from content, but it can't influence XHTML.

(2) fix nsHTMLCSSStyleSheet by adding the "anonymous element" to the AnonBoxRuleProcessorData, and using it to make nsHTMLCSSStyleSheet do the right thing for style attributes set on it.
Attachment #773223 - Flags: feedback?(dbaron) → feedback-
I think (2) is preferable, but I'd like bz to just double-check that he thinks it's sane
Flags: needinfo?(bzbarsky)
Seems sane to me, yes.
Flags: needinfo?(bzbarsky)
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #13)
> I think (2) is preferable [...]

I agree. We should not produce new HTML forms element that are not fully stylable.
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #12)
> (2) fix nsHTMLCSSStyleSheet by adding the "anonymous element" to the
> AnonBoxRuleProcessorData, and using it to make nsHTMLCSSStyleSheet do the
> right thing for style attributes set on it.

Shouldn't it be "PseudoElementRuleProcessorData" which should have an extra "mPseudoElement" instead of AnonBoxRuleProcessorData?
And so implement something in nsHTMLCSSStyleSheet::RulesMatching(PseudoElementRuleProcessorData*), which is empty currently?

Also, what is the "right thing" to do to have the style attributes taken into account?

I tried to do something similar to RulesMatching(ElementRuleProcessorData*) in RulesMatching(PseudoElementRuleProcessorData*) using a new "mPseudoElement" I've added in PseudoElementRuleProcessorData (set in ResolvePseudoElementStyle which is slightly changed to also take the pseudo element) but it doesn't work.
Do you think something else should be changed elsewhere?
(And, yes, PseudoElement rather than AnonBox is correct.)
Here is an updated version of the previous patch.
I've implemented RulesMatching(PseudoElementRuleProcessorData*), and add a mPseudoElement to PseudoElementRuleProcessorData, which is NULL by default (that way I didn't have to modify every call to ResolvePseudoElementStyle, as it's no useful to have this new param filled everywhere; I guess it's OK, at least for now).
In nsColorControlFrame, I'm passing mColorContent (the pseudo element) to ResolvePseudoElementStyle.

The problem is that ResolvePseudoElementStyle is also called by RestyleManager. In RestyleManager, I have access to the element, but I don't know how to retrieve the corresponding pseudo element.
That is actually my problem.

I haven't checked in more details, so I don't know if there is other generic places where ResolvePseudoElementStyle is called and where we should also be able to get the pseudo element, but I guess there are a few.
Attachment #784646 - Flags: feedback?(dbaron)
Comment on attachment 784646 [details] [diff] [review]
Attempt to have pseudo element working

Sorry, it looks like I attached the wrong patch :(
I will attach the correct patch asap.
I guess with the patch it will be clearer for you to understand what I mean in my previous comment.
Attachment #784646 - Attachment is obsolete: true
Attachment #784646 - Flags: feedback?(dbaron)
Here is the correct patch.
Attachment #785092 - Flags: feedback?(dbaron)
OK, I managed to have it working. Actually we use the parent element in Restylemanager::RestyleSelf, and we have access to the current element, so this is perfect :)
Everything works as expected for me, but I would like to have your feedback about this.
I surrounded my new code related to this in RestyleManager.cpp and nsHTMLCSSStyleSheet.cpp with checks on the pseudo type for now. But I don't think this should be specific to moz_color_swatch pseudo, and this seems to work fine if we do the same without this check.
Attachment #785092 - Attachment is obsolete: true
Attachment #785092 - Flags: feedback?(dbaron)
Attachment #786554 - Flags: feedback?(dbaron)
So part of the problem here is that there are two different sorts of pseudo-elements.  There are some, like ::before and ::after and ::first-letter and ::first-line, that exist only when we have styles for them.  (Though ::before and ::after get their own content node, whereas ::first-line and ::first-letter don't even get that.)  Then there are others, like most of the form pseudo-elements, I believe, that represents things that are always created.

We can really only offer the ability to have this sort of styling for pseudo-elements that are always created.

So I think the first step is to add a bit to nsCSSPseudoElementList.h and nsCSSPseudoElements.h separating those pseudo-elements.  Or perhaps it should be a bit for supports-style-attribute, with a comment saying that it can only be set on pseudo-elements of that type.  ResolvePseudoElementStyle and ProbePseudoElemenTstyle should then assert that their aPseudoElement parameter (please rename from aElement) is non-null if and only if the pseudo is one that has said bit.  nsHTMLCSSStyleSheet::RulesMatching could then also check that bit.

nsHTMLCSSStyleSheet::RulesMatching probably also doesn't need the SMIL bits.


Then, as far as getting restyling to work, RestyleManager::RestyleSelf would need to check the bit around its call to ResolvePseudoStyleContext, and pass mFrame->GetContent()->AsElement() (which is the pseudo-element content; note that |element| is something different -- see the function that computes it) when the bit says to do so.
Above comment was written before seeing comment 22.
Comment on attachment 786554 [details] [diff] [review]
Color input: layout part with pseudo element working (1st)

This is the right track, but comment 22 still stands:  use a bit instead of special-casing :-moz-color-swatch, I think I still prefer mFrame->GetContent()->AsElement() over mContent->AsElement(), remove the SMIL stuff, and rename the parameter.
Attachment #786554 - Flags: feedback?(dbaron) → feedback+
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #25)
> use a bit instead of special-casing :-moz-color-swatch,

I used the special-casing moz-color-swatch just as a sanity check for now. I thought we eventually want to have the same code for all pseudo elements.
Do you think we need we really need the additional bit? Why not having a check like
"mFrame->GetContent() &&  mFrame->GetContent()->IsElement() ? mFrame->GetContent()->AsElement() : nullptr"?

A bit sounds cleaner, but testing directly if the element exists sounds safer to me: we will not accidentally try to unref null objects or cast them to element while they are not, while in the meantime we will have this new feature available to all elements which can support it.

That's just my point of view, but if you think it's better to have a bit, I'm fine with this.
I guess we want something like "CSS_PSEUDO_ELEMENT_IS_CSS2" here, with a corresponding method (like "IsCSS2PseudoElement")?
Do you think we will need this new bit only for "HTML5 Forms pseudo elements", or more?
Do you have any idea regarding the new bit name?
Flags: needinfo?(dbaron)
(In reply to Arnaud from comment #26)
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #25)
> > use a bit instead of special-casing :-moz-color-swatch,
> 
> I used the special-casing moz-color-swatch just as a sanity check for now. I
> thought we eventually want to have the same code for all pseudo elements.
> Do you think we need we really need the additional bit? Why not having a
> check like
> "mFrame->GetContent() &&  mFrame->GetContent()->IsElement() ?
> mFrame->GetContent()->AsElement() : nullptr"?

Because it doesn't actually make sense for all pseudo-elements; only some pseudo-elements have a persistent content node.  For some, the content node is dynamically created or destroyed depending on the styles.

> A bit sounds cleaner, but testing directly if the element exists sounds
> safer to me: we will not accidentally try to unref null objects or cast them
> to element while they are not, while in the meantime we will have this new
> feature available to all elements which can support it.

In addition to :before and :after where it would produce weird results if it ever did anything, it would be worse for ::first-letter and ::first-line where there is no extra element for the pseudo-element, and you'd just get the original element, which would produce incorrect results.

> That's just my point of view, but if you think it's better to have a bit,
> I'm fine with this.
> I guess we want something like "CSS_PSEUDO_ELEMENT_IS_CSS2" here, with a
> corresponding method (like "IsCSS2PseudoElement")?

Yes.

> Do you think we will need this new bit only for "HTML5 Forms pseudo
> elements", or more?

Well, we only need it for those where we actually have styles in the style attribute, which is currently only :-moz-color-swatch.  But we could extend it further, probably to roughly that set, but for each extension we'd need to (a) check that it obeys the necessary invariants and (b) change the initial way we resolve the pseudo-element style in the frame construction code (thus the need for an assertion to check tings are being done correctly).  I think it's probably best to just do :-moz-color-swatch for now, and leave a comment explaining how it can be extended.

> Do you have any idea regarding the new bit name?

CSS_PSEUDO_ELEMENT_SUPPORTS_STYLE_ATTRIBUTE ?
Flags: needinfo?(dbaron)
Attached patch Color input: layout part (5th) (obsolete) — — Splinter Review 
Here is a new patch, with the pseudo-element working ;)

Compared to previous patch, in addition of the changes we discussed, I've added a special "case NS_FORM_INPUT_COLOR:" in HTMLInputElement so the color picker will show up if it's selected and we press 'Enter' (I missed this when doing bug 875274, but it's not a big change anyway).

David, could you please review it? (or delegate it to someone else if you're busy and think someone else can take care of this)
Attachment #773223 - Attachment is obsolete: true
Attachment #786554 - Attachment is obsolete: true
Attachment #788429 - Flags: review?(dbaron)
(In reply to Mounir Lamouri (:mounir) from comment #8)
> Arnaud, I got an idea regarding how to push this part of the feature without
> making disabling <input type='color'> not working: I think you should not
> change forms.css with the layout patch. Basically, as long as we don't touch
> forms.css, we can safely keep everything behind the pref so we could land
> that patch and add the backends when they are ready. Then, when we have all
> the backends and the feature will be ready to ship, we could add the
> forms.css changes and remove the pref (given that, at that point, the pref
> would be useless). Do you think that could work? In other words, would the
> feature work if there is no stylesheet for the input type? Being ugly is
> okay as long as it is usable.

Sorry for the late answer (and for accidentally removing your needinfo flag request).

I tested this (i.e. using "setAttr(style)" instead of setting the CSS properties in the form.css) and indeed, it looks like it works (but it's ugly).

I don't think we can set CSS selectors this way, but I'm not sure we need to redefine them in form.css anyway, so maybe it can be a good solution if we want to ship this feature only on few platforms (i.e. those where a color picker has been implemented).
Status: NEW → ASSIGNED
Sorry to bother but how can I test this ? :)
Comment on attachment 788429 [details] [diff] [review]
Color input: layout part (5th)

>+  static bool PseudoElementSupportsStyleAttribute(nsIAtom *aAtom) {
>+    return PseudoElementHasFlags(aAtom, CSS_PSEUDO_ELEMENT_SUPPORTS_STYLE_ATTRIBUTE);
>+  }
>+
>+  static bool PseudoElementSupportsStyleAttribute(const Type aType) {
>+    MOZ_ASSERT(aType < ePseudo_PseudoElementCount);
>+    return PseudoElementHasFlags(GetPseudoAtom(aType), CSS_PSEUDO_ELEMENT_SUPPORTS_STYLE_ATTRIBUTE);
>+  }

This is a bit inefficient, since GetPseudoAtom() converts from an index into
the array to an atom, and then PseudoElementHasFlags searches the array to find
the index.

We should really:
 * change nsCSSPseudoElements::PseudoElementHasFlags and
   nsCSSPseudoElements::FlagsForPseudoElement and
   nsCSSPseudoElements::PseudoElementContainsElements to take
   nsCSSPseudoElements::Type instead of nsIAtom*
 * make nsComputedDOMStyle::GetPropertyCSSValueInternal call
   PseudoElementContainsElements with
   topWithPseudoElementData->GetPseudoType() instead of
   topWithPseudoElementData->GetPseudo()
 * fix nsCSSPseudoElements::IsCSS2PseudoElement to call
   GetPseudoType on the first parameter to the call of PseudoElementHasFlags

That should be a separate patch, though.


In nsStyleSet.cpp, please fix the 80th column violations.



r=dbaron on the layout/style/* changes other than forms.css.

I'm going to transfer the rest of the review request to dholbert in the hopes that that will be a faster way to get this review done; sorry for taking so long to get to this.
Attachment #788429 - Flags: review?(dholbert)
Attachment #788429 - Flags: review?(dbaron)
Attachment #788429 - Flags: review+
(I probably won't get to this review today; hoping to look into it on Monday.)
[quick review note per IRC: from a quick skim of the patch, I'm pretty sure the #ifdef ACCESSIBILITY chunks aren't needed]
(In reply to Daniel Holbert [:dholbert] from comment #32)
> (I probably won't get to this review today; hoping to look into it on
> Monday.)

OK, thanks a lot :)

The patch is probably not perfect, but I already had some comments from David and Mounir, so hopefully it's not that bad ;)

The only thing I'm not sure about is forms.css: I'm not sure we need the pseudo-classes definition for input[type="color"].
Also, as Mounir noticed in comment 8, I might be better to temporary set the CSS for input[type="color"] in nsColorControlFrame.cpp instead, so the CSS will not applied when dom.forms.color pref is false (and so we can enable this feature only on platforms where we have color picker implemented).
I tested and it works.
Comment on attachment 788429 [details] [diff] [review]
Color input: layout part (5th)

># HG changeset patch
># User Arnaud Bienner <arnaud.bienner@gmail.com>
># Date 1373408727 -7200
>#      Wed Jul 10 00:25:27 2013 +0200
># Node ID c0138fbbe98fce588f498ecdff054824896186b5
># Parent  ad0ae007aa9e03cd74e9005cd6652e544139b3b5
>[mq]: color_layout

nit: Please add a commit message.

>diff --git a/browser/devtools/styleinspector/css-logic.js b/browser/devtools/styleinspector/css-logic.js
>--- a/browser/devtools/styleinspector/css-logic.js
>+++ b/browser/devtools/styleinspector/css-logic.js

Looks like this file moved from /browser to /toolkit in bug 897275.  (If I edit the patch file to fix the path, it still applies correctly, though.)

>diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h
>--- a/content/base/src/nsGkAtomList.h
>+++ b/content/base/src/nsGkAtomList.h
>@@ -1784,16 +1784,17 @@ GK_ATOM(bulletFrame, "BulletFrame")
> GK_ATOM(frameSetFrame, "FrameSetFrame")
>+GK_ATOM(colorControlFrame, "colorControlFrame")
> GK_ATOM(gfxButtonControlFrame, "gfxButtonControlFrame")

This file is supposed to be in alphabetical order. Please move the new "colorControlFrame" line up a bit to its correct spot in the "c" section.

>+++ b/layout/forms/nsColorControlFrame.cpp
>+// Create the color area for the button.
>+// The frame will be generated by the frame constructor.
>+nsresult
>+nsColorControlFrame::CreateAnonymousContent(nsTArray<ContentInfo>& aElements)
>+{
>+  nsCOMPtr<nsIDocument> doc = mContent->GetDocument();

GetDocument is deprecated. This should probably be GetCurrentDoc.
For reference, see:
http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIContent.h?rev=c7dcecd40428#110

>+  NS_NewHTMLElement(getter_AddRefs(mColorContent),
>+                    nodeInfo.forget(), mozilla::dom::NOT_FROM_PARSER);
>+
>+  if (!mColorContent) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }

Instead of this null-check, you should be capturing the return value of this NS_NewHTMLElement, and doing NS_ENSURE_SUCCESS on it, like we do e.g. here:
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsMeterFrame.cpp?rev=19e67876d0cd#71

>+void
>+nsColorControlFrame::AppendAnonymousContentTo(nsBaseContentList& aElements,
>+                                                  uint32_t aFilter)

Fix indentation.

>+NS_QUERYFRAME_HEAD(nsColorControlFrame)
>+  NS_QUERYFRAME_ENTRY(nsIAnonymousContentCreator)
>+NS_QUERYFRAME_TAIL_INHERITING(nsBlockFrame)

Please move this boilerplate higher up, towards the top of the file, right after the NS_IMPL_FRAMEARENA_HELPERS line, to keep macro-boilerplate as quarantined away from "real" code as possible. :) 

>+nsString
>+nsColorControlFrame::GetColor() const
>+{
>+  nsString aColor;

The prefix "a" means "argument" -- so this should be named "color" or "colorStr".

ALSO: aColor (or colorStr or whatever) should be a nsAutoString, not a nsString. (nsAutoString like nsString, but it brings along a stack-allocated buffer, which makes it more efficient as a local variable. (avoiding heap allocation))

ALSO: this function should probably be morphed into "void nsColorControlFrame::AppendColorString(nsAString& aStr)" -- i.e. it should directly append to its outparam, rather than returning a nsString which we know the caller is going to append.   That'll let us use nsAutoString in the caller, as well, and avoid heap allocation / string-copying as much as possible.

>+nsresult
>+nsColorControlFrame::UpdateColor()
>+{
>+  return mColorContent->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
>+      NS_LITERAL_STRING(" background-color: ") + GetColor() + NS_LITERAL_STRING("; "), true);

So, per above, this should start out by declaring something like:
  nsAutoString str = NS_LITERAL_STRING("background-color: ");
and then pass "str" to AppendColorString(), and then pass "str" to SetAttr.

(I think you can skip the terminal "; " if you want, since we're only setting one property here.

>+NS_IMETHODIMP
>+nsColorControlFrame::AttributeChanged(int32_t  aNameSpaceID,
>+                                      nsIAtom* aAttribute,
>+                                      int32_t  aModType)
>+{
>+  NS_ASSERTION(mColorContent, "The color div must exist");
>+
>+  // If the value attribute is set, update the color box
>+  if (nsGkAtoms::value == aAttribute) {
>+    UpdateColor();
>+  }

Nit: your "if" check there also needs to check that aNameSpaceID == kNameSpaceID_None. (see e.g.  nsMeterFrame::AttributeChanged for reference)

>+void
>+nsColorControlFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
>+                                  const nsRect&           aDirtyRect,
>+                                  const nsDisplayListSet& aLists)

Fix indentation of those last 2 lines.

>+++ b/layout/forms/nsColorControlFrame.h
>+class nsColorControlFrame MOZ_FINAL : public nsBlockFrame,
>+                                      public nsIAnonymousContentCreator
>+{
>+public:
>+  nsColorControlFrame(nsStyleContext* aContext);
>+
>+  void DestroyFrom(nsIFrame* aDestructRoot) MOZ_OVERRIDE;
>+
>+  NS_DECL_QUERYFRAME_TARGET(nsColorControlFrame)

I'm pretty sure you only need to declare this as a QueryFrame target if you:
 (a) need to do_QueryFrame to it.
 (b) have an ENTRY() for this frame class in your NS_QUERYFRAME_* block in the .cpp file.

Neither of those are the case, so I think you can drop this line.

You should probably also declare the constructor in the "private" section at the bottom, and declare NS_ColorControlFrame as a friend function, so that it's the *only* thing that can allocate an instance of this class.

See e.g. this line of nsBlockFrame for sample "friend" usage:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.h?rev=f10d31a0f7b4#135

>+  // nsIAnonymousContentCreator
>+  nsresult CreateAnonymousContent(nsTArray<ContentInfo>& aElements) MOZ_OVERRIDE;
>+  void AppendAnonymousContentTo(nsBaseContentList& aElements,
>+                                        uint32_t aFilter) MOZ_OVERRIDE;

Fix indentation.

>+  // nsIFrame
>+  NS_IMETHOD AttributeChanged(int32_t         aNameSpaceID,
>+                              nsIAtom*        aAttribute,
>+                              int32_t         aModType) MOZ_OVERRIDE;

De-indent these super-indented arguments, unless you're trying to line them up with something in particular.

>+  bool IsLeaf() const MOZ_OVERRIDE { return true; }
>+  nsIFrame* GetContentInsertionFrame() MOZ_OVERRIDE;

Nit: Mark these as "virtual", for clarity. (They're declared as virtual in the ancestor class, which means "virtual" is already implied here. But per a recent m.d.platform thread, I think the preferred style is to include an explicit "virtual" in the child class as well.)

(This applies to all the other MOZ_OVERRIDE methods in this file, too; they should all be marked as virtual, unless they're declared as NS_IMETHOD, which is a macro that includes "virtual" already.)

>+  void BuildDisplayList(nsDisplayListBuilder*   aBuilder,
>+                                const nsRect&           aDirtyRect,
>+                                const nsDisplayListSet& aLists) MOZ_OVERRIDE;
>+

Fix weird indentation on those last two lines (both of the type and of the arg-names).  The types should all be aligned, and the arg-names should either all be aligned or all be 1 space away from the type.

>+++ b/layout/reftests/forms/input/color/block-invalidate-ref.html

Please name these "block-invalidate-1.html" and "block-invalidate-1-ref.html".  It's very likely that for at least some of your tests, you (or someone else down the line) will want to add additional variants, and the numbered naming scheme makes this straightforward.

(You can fix this pretty quickly by opening the patch file in an editor and doing a search-and-replace, e.g. to replace all instances of "block-invalidate" with "block-invalidate-1" in your patch file.)

>+++ b/layout/reftests/forms/input/color/block-invalidate.html
>@@ -0,0 +1,16 @@
>+<!DOCTYPE html>
>+<html class='reftest-wait'>
>+  <link rel='stylesheet' type='text/css' href='style.css'>

Hmm... So it looks like all of your reftests use "style.css", which applies its own theming to the <input> element. That's handy to test, but it's even more important to have tests that check the default theming, since that's what authors are going to get (absent any of their own tweaks). Could you add some tests that don't apply any special theming to the <input> element, as well?

>+  <script>
>+    function loadHandler() {
>+      setTimeout(function() {
>+        var p = document.getElementsByTagName('input')[0];
>+        p.value = '#00ff00';
>+        document.documentElement.className = '';
>+      }, 0);
>+    }
>+  </script>
>+  <body onload="loadHandler();">

setTimeout is frowned upon in our test suites (even with "0").  A better solution is to listen for MozReftestInvalidate -- that's a test-harness-only event that gets fired after we've painted and are ready for dynamic tweaks.  (search MXR for that token for sample usage)

>+++ b/layout/reftests/forms/input/color/input-color-ref.html
[...]
>+</html>
>+

nit: this file has a bonus blank line at the end -- please remove it.

>+++ b/layout/reftests/forms/input/color/reftest.list
>@@ -0,0 +1,7 @@
>+
>+default-preferences pref(dom.forms.color,true)

Drop the blank line at the beginning of this file. (or, populate it with a comment. :))

That's a lot to fix (albeit mostly minor stuff), so I'll hold off on granting r+ for now.  Would you mind fixing the above (and comment 31 & comment 33) and re-posting, for a final review? (I may add a few more things at that point, but probably not much.)  Thanks!
(In reply to Daniel Holbert [:dholbert] from comment #35)
> ALSO: this function should probably be morphed into "void
> nsColorControlFrame::AppendColorString(nsAString& aStr)" -- i.e. it should
> directly append to its outparam, rather than returning a nsString which we
> know the caller is going to append.   That'll let us use nsAutoString in the
> caller, as well, and avoid heap allocation / string-copying as much as
> possible.

I used GetColor to make the code a bit clearer, but, thinking again about this, I'm not sure it's worth to have this method, as it's very simple and not intended to be used anywhere else.
I should probably move the interesting code (2 lines) directly into "UpdateColor" method.
I would prefer this instead of having a "AppendColorString" which sounds less readable for me (I don't really like functions with outparam when they are not needed).
Would this be OK for you?

> Hmm... So it looks like all of your reftests use "style.css", which applies
> its own theming to the <input> element. That's handy to test, but it's even
> more important to have tests that check the default theming, since that's
> what authors are going to get (absent any of their own tweaks). Could you
> add some tests that don't apply any special theming to the <input> element,
> as well?

style.css has rules only for "div.input-color" and "div.input-color-swatch" so those div are supposed to look like a native <input type=color> (and so my tests, which are based on display comparison, can check if input color is broken). It doesn't apply any additional style.
But it looks like I included this file in too many places. I can remove it where it's not needed if you want.

> setTimeout is frowned upon in our test suites (even with "0").  A better
> solution is to listen for MozReftestInvalidate -- that's a test-harness-only
> event that gets fired after we've painted and are ready for dynamic tweaks. 
> (search MXR for that token for sample usage)

I looked at existing code for examples and found this (maybe it has been removed since though). I will try MozReftestInvalidate :)

> That's a lot to fix (albeit mostly minor stuff), so I'll hold off on
> granting r+ for now.  Would you mind fixing the above (and comment 31 &
> comment 33) and re-posting, for a final review? (I may add a few more things
> at that point, but probably not much.)  Thanks!

Thanks for taking some time to do the review :)

One last thing, is the form.css fine for you?
As I explained in comment 34, it would be better to set these CSS rules from nsColorControlFrame directly. If the CSS rule for input type color is OK for you, I will move this to nsColorControlFrame  within my next patch.
As I said, I don't think we need the new pseudo-classes.
(In reply to Arnaud Bienner from comment #36)
> I used GetColor to make the code a bit clearer, but, thinking again about
[...]
> I should probably move the interesting code (2 lines) directly into
> "UpdateColor" method.

That sounds good.

> style.css has rules only for "div.input-color" and "div.input-color-swatch"
[...]
> But it looks like I included this file in too many places. I can remove it
> where it's not needed if you want.

Ah -- yes, I misread. Yes, please only include it where you need it. (reference cases, I think?)

> > setTimeout is frowned upon in our test suites (even with "0").  A better
> 
> I looked at existing code for examples and found this (maybe it has been
> removed since though). I will try MozReftestInvalidate :)

(You probably just ran across old tests, from before we had MozReftestInvalidate available to us.)

> One last thing, is the form.css fine for you?

Not sure - I haven't looked into that / comment 34 in detail, but I will. [off to a meeting right now; I'll get back to you on that in a bit]
[also -- I just noticed -- you ll need to add your reftest.list file to the parent directory's reftest.list:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/forms/input/reftest.list
...or else your tests will be stranded & will never get run. :) ]
A few more notes:
 - All of the reftests except for block-invalidate.html currently fail on my system (on Linux), due to what looks like sizing differences. Here's a try run showing the issues: https://tbpl.mozilla.org/?tree=Try&rev=6ba382855452

 - It looks like the list in "css-logic.js" is currently in alphabetical order; please adjust your added line there, to preserve alphabetical ordering.

Getting back to your forms.css question -- I agree with mounir that it'd be nice to avoid affecting the behavior of builds with the pref turned off, as long as we have the pref off-by-default.  So, I agree that we shouldn't touch forms.css yet.  (That means we'll get the default <input> styling, including "-moz-appearance: textfield;", which is kind of unfortunate, but maybe not horrible if we're mostly just trying for a proof-of-concept-UI right now...  And it sounds like you have a workaround to make it still usable, at the end of comment 34?)
(Also: With your patch applied & the pref toggled, if I load one of the testcases and click the widget a bunch of times, I get a bunch of color-picker dialogs. Even with one or more of the dialogs up, I can continue clicking the widget to spawn even more dialogs. We should fix that, though probably not as part of this bug here.  Do you know if that's already tracked anywhere? and if it's not, perhaps you could file a followup on it?)
Also: jwatt points out to me that, per dbaron/roc/bz in bug 664968 comment 2 through 6, <input> frames probably shouldn't be deriving from nsBlockFrame.

(I previously thought this might be OK because nsFileControlFrame derives from nsBlockFrame, but I'll defer to dbaron/roc/bz's judgement)
 
So this probably should inherit from nsContainerFrame instead of from nsBlockFrame, which means you'll have to provide a ::Reflow() method.  IIRC, nsRangeFrame and nsProgressFrame might have reasonable reflow methods that you can crib from.
(In reply to Daniel Holbert [:dholbert] from comment #41)
> Also: jwatt points out to me that, per dbaron/roc/bz in bug 664968 comment 2
> through 6, <input> frames probably shouldn't be deriving from nsBlockFrame.

I don't think that's an absolute -- it depends on how much of nsBlockFrame it actually makes use of.
I don't think we're making any more use of nsBlockFrame here than we are on bug 664968. (in particular, we don't use the splittability, and we don't use multiple lines).

Right now, the rendering is just a button with a colorful rect on it.
(...which makes me wonder if this should actually derive from nsGfxButtonControlFrame, assuming we're sure it's supposed to render sort of like <input type="button">?)
> [also -- I just noticed -- you ll need to add your reftest.list file to the
> parent directory's reftest.list:
> http://mxr.mozilla.org/mozilla-central/source/layout/reftests/forms/input/
> reftest.list
> ...or else your tests will be stranded & will never get run. :) ]

Ah ok, thanks for letting me know :)
I just ran them manually after I wrote them. I didn't know I have to add them here (I thought they will be discovered in some way as they are in reftests dir).
I remember I changed the form.css later, and forgot to update the corresponding tests, so indeed they should fail now :/ I will fix that.

> (Also: With your patch applied & the pref toggled, if I load one of the
> testcases and click the widget a bunch of times, I get a bunch of
> color-picker dialogs. Even with one or more of the dialogs up, I can
> continue clicking the widget to spawn even more dialogs. We should fix that,
> though probably not as part of this bug here.  Do you know if that's already
> tracked anywhere? and if it's not, perhaps you could file a followup on it?)

With Mounir we wanted to be as close as possible to the classic system behavior for each OS on which we implemented a widget. i.e. making the widget popup modal or not.
Making the popup modal would fix the issue, but IMHO we should make windows modal when it's not really needed.
For systems where the popup isn't modal, maybe we should bring back the old picker to front instead of opening a new one?
This can be discussed in a follow up bug indeed. Or in bug 885996? or in one of the widget bugs (bug 875753?)

> (...which makes me wonder if this should actually derive from
> nsGfxButtonControlFrame, assuming we're sure it's supposed to render sort of
> like <input type="button">?)

When I started to work on this, I made nsColorControlFrame inherit from nsHTMLButtonControlFrame, but Mounir suggested it might not be a good idea (bug 547004 comment 67). If I remember right, he suggested to use nsBlockFrame as it has a default Reflow method which fit our needs. Personally, I'm a bit reluctant to rewrite a Reflow method if the inherited one is working well.
Maybe we should have a simple container class with a default Reflow method? as bz suggested in bug 664968 comment 5. If so, maybe this can be done in a follow up bug?

IMO this input type should look like a button (which it does thanks to CSS) but it doesn't share anything else with a classic button I think. Mounir suggested it to be something like:
<div>
  <div></div>
</div>
And it sounds like a good idea to me.
I'm not sure what are the advantages of inheriting from nsGfxButtonControlFrame or nsHTMLButtonControlFrame.
Blocks: 917917
I think bug 917917 can be referenced in bug 547004 instead, which is the "master bug" for input type color, as bug 917917 isn't related to the layout.
No longer blocks: 917917
(In reply to Arnaud Bienner from comment #45)
> Ah ok, thanks for letting me know :)
> I just ran them manually after I wrote them.

No problem. :) For future reference -- if you'd like to run the tests yourself, you can run this (from your sourcedir):
 ./mach reftest layout/reftests/forms/input/color
(That'll work regardless of whether you've added them to the parent manifest, because you're telling it where they are)

> For systems where the popup isn't modal, maybe we should bring back the old
> picker to front instead of opening a new one?

That would solve the accidental-extra-clicks-spawning-extra-dialogs problem, yeah. In any case, I filed bug 917917 on this; let's figure this out over there.  (And per comment 46, I made it depend on bug 547004 instead of on this bug.)

> When I started to work on this, I made nsColorControlFrame inherit from
> nsHTMLButtonControlFrame, but Mounir suggested it might not be a good idea
> (bug 547004 comment 67).
> If I remember right, he suggested to use
> nsBlockFrame as it has a default Reflow method which fit our needs.

Ah -- and more importantly, he said that we want it to be able to use system-specific colorpicker widgets (which I think he was implying do *not* necessarily look like buttons, maybe?)

> IMO this input type should look like a button (which it does thanks to CSS)
> but it doesn't share anything else with a classic button I think.

So: assuming that it *is* going to look like a button (but with button-text replaced with something colorful), then I think inheriting from nsHTMLButtonControlFrame would make sense.

If not, we might still be able to make it work... I'm not sure. If not, the "inherit from container class with sane reflow method" would be better.

> Personally, I'm a bit reluctant to rewrite a Reflow method if the inherited
> one is working well.

You won't need to write one, if you inherit from nsHTMLButtonControlFrame. :)

> IMO this input type should look like a button (which it does thanks to CSS)
> but it doesn't share anything else with a classic button I think. Mounir
> suggested it to be something like:
> <div>
>   <div></div>
> </div>

So -- if you were implementing this in pure CSS, then that would probably make sense to construct something like that, with styled <divs>.  However, given that you're adding a custom frame class, we can be a bit smarter about that frame class's behavior. And per bug 664968 comment 2 through 6, nsBlockFrame is kind of overkill for what you need here. (And kind of what you *don't* want, in some ways.)

Also: frametree-wise, a button actually looks a lot like those nested <div>s, under the hood. (It's got the button frame, which contains an anonymous "-moz-button-content" node inside of it, which paints the button-text & could play the role of the color-swatch in this case.)  So we're not too far off from that idea.

> I'm not sure what are the advantages of inheriting from
> nsGfxButtonControlFrame or nsHTMLButtonControlFrame.

Good question -- I think nsHTMLButtonControlFrame would probably make more sense, now that you mention it. I was initially assuming you'd want the GFX one, since it's a subclass that we use for button-flavored <input> elements (submit/reset/filecontrol).  But on closer inspection, it looks like the main functionality it adds is the ability to have a default text label ("submit"/"reset"), which you don't need here. So you probably would want to inherit from nsHTMLButtonControlFrame.
So here's what Mounir said in bug 547004 comment 67 (the comment Arnaud mentioned, recommending against nsHTMLButtonControlFrame):
> I'm not sure that we should have
> nsColorControlFrame inherits from nsHTMLButtonControlFrame. I think the
> color frame should be a ContainerFrame with one anonymous child that would
> be a simple div. [...]
> The outer div would be the nsColorControlFrame and the inner div the
> anonymous element.
> The outer div would look like a button and the inner div would be a plain
> div reflecting the current value

Based on the requirements listed there, nsHTMLButtonControlFrame actually sounds like a pretty ideal match.  The nsHTMLButtonControlFrame is the outer ContainerFrame (which looks like a button), and it has a single anonymous node inside it ("-moz-button-content") that you can style to look like whatever you want.
(In reply to Daniel Holbert [:dholbert] from comment #48)
> and it has a single anonymous
> node inside it ("-moz-button-content") that you can style to look like
> whatever you want.

I don't see how this "-moz-button-content" element is created.
It seems there is nothing like "NS_NewHTMLElement" I've used to create the pseudo element, either in nsHTMLButtonControlFrame nor in nsButtonFrameRenderer (which is used by nsHTMLButtonControlFrame).
Also, in the first patch I made, nsColorControlFrame inherited from nsHTMLButtonControlFrame but created the pseudo element anyway.

If really I don't need to create a new pseudo element, do you know how to can I "co-opt" it, so it will be stylable for users using "-moz-color-swatch" selector, and how can I change its "style" property to change its background color?
Comment on attachment 788429 [details] [diff] [review]
Color input: layout part (5th)

Marking patch r-, only to indicate that there's still a good chunk of stuff left to do before it'll be landing-ready.

(I was going to just clear the review request and mark it as "feedback+", but I don't want the "r=dbaron" flag [which just covers layout/style] to mislead any eager patch-landers to thinking this is ready to land.)

Thanks for your work on this! Looking forward to the next patch-version. :)
Attachment #788429 - Flags: review?(dholbert)
Attachment #788429 - Flags: review-
Attachment #788429 - Flags: feedback+
(In reply to Arnaud Bienner from comment #49)
> I don't see how this "-moz-button-content" element is created.

It looks like technically there's no actual element created -- it's just an anonymous box (i.e. frame), whose content node is also the <input>/<button> element, but with its own style context (addressed via the moz-button-content pseudo-class).

This gets hooked up for "button" here:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=5294bf1a2140#3261
(and it gets similarly hooked up lower down for <input type="reset">, with NS_NewGfxButtonControlFrame instead of HTML)

> Also, in the first patch I made, nsColorControlFrame inherited from
> nsHTMLButtonControlFrame but created the pseudo element anyway.

I suspect we don't want to do that. Then we'd have two anonymous children, with one unused, which is inefficient.

> If really I don't need to create a new pseudo element, do you know how to
> can I "co-opt" it, so it will be stylable for users using
> "-moz-color-swatch" selector

I'd expect that you could style it in forms.css using something like input[type=color]::-moz-button-content or something like that... (and users would be able to style it similarly. Not sure, though.

> and how can I change its "style" property

Hmm. That's a good question. I was thinking you'd do the same thing you did for the color-swatch node, but now there's no dedicated anymous node, so that might be tricky...  This is probably a question for dbaron or perhaps bz.  dbaron, do you know if there's a way to dynamically change the style of an anonymous box (for moz-button-content in this case) with no dedicated content node?
Flags: needinfo?(dbaron)
My quick attempts at styling -moz-button-content (copypasting the selector from forms.css into a <style> block in an HTML document) are failing with "Unknown pseudo-class or pseudo-element '-moz-button-content'.  Ruleset ignored due to bad selector."  Not sure if it's impossible to style, or if I'm just doing it wrong.

BUT -- good news -- from some quick playing around with e.g....
  <button><span style="background: purple">ohai</span></button
...I suspect any nodes (anonymous or otherwise) inside your <button> would be children of -moz-button-content in the box tree, and would be stylable.

So I suspect your old "inherit from nsHTMLButtonControlFrame but create the pseudo element anyway" strategy is sensible. (and then you can set the style attribute and allow users to modify the style in the same way as your attached patch does.)
(er, by "inside your <button>", I meant "inside your <input type="color"/>")
(You'll need to remove your current patch's nsCSSFrameConstructor.cpp addition, and instead add a braced-block like we have for NS_FORM_INPUT_SUBMIT etc., here:
  http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=5294bf1a2140#3327
but with s/NS_NewGfxButtonControlFrame/NS_NewNewColorControlFrame/)
(In reply to Daniel Holbert [:dholbert] from comment #54)
> (You'll need to remove your current patch's nsCSSFrameConstructor.cpp
> addition, and instead add a braced-block like we have for
> NS_FORM_INPUT_SUBMIT etc., here:
>  
> http://mxr.mozilla.org/mozilla-central/source/layout/base/
> nsCSSFrameConstructor.cpp?rev=5294bf1a2140#3327
> but with s/NS_NewGfxButtonControlFrame/NS_NewNewColorControlFrame/)

Yep probably: that's what I did in my first patch which inherited from nsHTMLButton.

so I can for from "inherit from nsHTMLButtonControlFrame but create the pseudo element anyway"? Or do you want to wait for David's feedback?
I'd like to get a sanity-check from David before fully committing to sending you down that path. :)

(dbaron: see comments 51-52 in particular.)
Talking to bz about this right now. He agrees nsHTMLButtonFrame is a good candidate to inherit from.

He brings up a good question, though: Do we really need the color-swatch to be stylable by users/authors? (and what aspects of it do we want them to be able to style?)
Flags: needinfo?(dbaron)
(In reply to Daniel Holbert [:dholbert] from comment #57)
> He brings up a good question, though: Do we really need the color-swatch to
> be stylable by users/authors? (and what aspects of it do we want them to be
> able to style?)

Yes we should IMHO. Is there any reason why we should not?
Mounir said in comment 15 this should be part of the first step of the implementation, and gave some details in bug 547004 comment 67.
Now I spent a lot of time working on this (mostly fixing the changes David reviewed, particularly in nsHTMLCSSStyleSheet), I would be a bit sad to drop all the work I've done :p
Also, I think it's worth to allow users to change some CSS properties (e.g. border margin, for those who will want a small color button, but want the colorful rect to be bigger in this case).
(In reply to Arnaud Bienner from comment #58)
> Yes we should IMHO. Is there any reason why we should not?

Just to avoid introducing unnecessary complexity (& code-maintenance burden).

> Mounir said in comment 15 this should be part of the first step of the
> implementation, and gave some details in bug 547004 comment 67.

Hmm... He said there:
   we need to make sure the element is stylable. That means
   that we should provide a pseudo-element for the inner-div.
   [...] The inner-div might have a few styling restrictions
   (likely the same as <progress> inner-div) and will not allow
   the authors to change its 'background' property for obvious
   reasons.

I don't understand what benefit authors get from being able to style it, though. It seems like that inner div's only purpose is to show a particular background color, and we can take care of that part internally without needing a pseudo element.

> Now I spent a lot of time working on this (mostly fixing the changes David
> reviewed, particularly in nsHTMLCSSStyleSheet), I would be a bit sad to drop
> all the work I've done :p

I know :-/ I'm sorry about that. Maybe it is necessary; I just want to make sure, and right now I'm not convinced.

> Also, I think it's worth to allow users to change some CSS properties (e.g.
> border margin, for those who will want a small color button, but want the
> colorful rect to be bigger in this case).

So you're talking about sizing of the swatch inside the button there. I'm pretty sure authors can already control that by adjusting "padding" on the <input> element itself.  e.g. if they want the swatch to occupy nearly all of the button, they can set padding to 0; if they want it to occupy a tiny part of the button, they can padding to something larger.

Now, if they want an actual *border* around the swatch (inside the button), *then* I think they would need a pseudo-element. However, I'm not convinced that letting authors set arbitrary borders inside of the button is an important enough use-case to merit adding this pseudo-element. (And that's the best use case I can think of for what authors would actually want to style.)

One option would be to split your pseudo-element code into a followup patch, and start out assuming that this is *not* a pseudo-element -- and then we can switch it over to be a pseudo-element in a subsequent bug/patch, if that ends up being important.  I think that subsequent patch wouldn't need to change the actual frame class too much, and it'd be a relatively atomic change to make, if mounir (or someone else) has a convincing argument for it.
(In reply to Daniel Holbert [:dholbert] from comment #59)
> One option would be to split your pseudo-element code into a followup patch,
> [...] I think that subsequent patch wouldn't need to
> change the actual frame class too much, and it'd be a relatively atomic
> change to make, if mounir (or someone else) has a convincing argument for it.

("somebody else" there includes you, of course. :) I was just mentioning mounir since he'd made that initial pseudo-element request; given that he's on vacation at the moment, I don't know exactly what he had in mind, but in the off chance that he's monitoring requests in bugmail, I'll tag him for needinfo. mounir: see comment 57 - 59.)
Flags: needinfo?(mounir)
OK - I just chatted w/ dbaron on IRC about this - he's comfortable with the style-system code in this patch, and he thinks it's probably worth exposing the swatch to be author-stylable as long as there's no harm in doing so. I'll defer to his judgement on that.

SO: let's go ahead with:
 (a) making your frame inherit from nsHTMLButtonFrame, and
 (b) using a moz-color-swatch pseudo element

(aside: it'd be worth including a testcase that asserts that authors can't use the pseudo-element to change the background color of the swatch away from the current colorpicker value-reading)

(also: RE keeping forms.css unstyled, per comment 34 / comment 8 -- I think you should be fine adding input[type="color"]::-moz-color-swatch {  } rules there, since that won't affect behavior in any builds that have this preffed off (since those builds won't have any instances of that pseudo-element). You'll just want to avoid adding any rules for input[type=color] itself to forms.css.)
Flags: needinfo?(mounir)
Inheriting from nsHTMLButtonFrame will pull a lot of quirks that exist in <button> for historic reasons. It is not clear to me why we want to suffer with that while the internal representation should mostly be a <div><div></div></div> with some styling.
Flags: needinfo?(dholbert)
(In reply to Mounir Lamouri (:mounir) from comment #62)
> Inheriting from nsHTMLButtonFrame will pull a lot of quirks that exist in
> <button> for historic reasons. It is not clear to me why we want to suffer
> with that

What quirks are those?  Are you sure they're quirks of nsHTMLButtonFrame, and not <button>-specific?

> while the internal representation should mostly be a
> <div><div></div></div> with some styling.

Basically, nsBlockFrame is more powerful/heavy/complex than what we need in this case. It'll work, but it might be a case of hammering a screw in with a sledgehammer, when it looks like we have a screwdriver in our toolbox.  And if we're making a point of avoiding inheriting from nsBlockFrame for other form controls (as in bug 664968), we should be consistent about that.

Basically, if we're making a new button-themed <input> control, it seems sensible to me that we'd want to share the rendering object that we already use use for other button-themed <input> controls (like "submit" and "reset"), unless there are clear reasons why that won't work.
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #63)
> Basically, nsBlockFrame is more powerful/heavy/complex than what we need in
> this case. It'll work, but it might be a case of hammering a screw in with a
> sledgehammer, when it looks like we have a screwdriver in our toolbox.

From what I understand, it's more like we don't have the right tool in our toolbox (looking at comments here and at bug 664968 comment 5).
Attached patch Color input: layout part (6th) (obsolete) — — Splinter Review 
There was quite a lot of (small) changes to do, so here is a new patch with everything applied *except* the "nsColorControlFrame inherits from nsBlockFrame" problem.
It's a bigger change, so I will modify this a second step as I would like to have everything else r+ so we can focus on this.

I'm making the previous attachment 788429 [details] [diff] [review] obsolete, but don't forget some parts were r+ by David :)
Attachment #809518 - Flags: review?(dholbert)
And here is the interdiff.
I don't think you need to review attachment 809518 [details] [diff] [review] actually: this one should be enough.
Attachment #788429 - Attachment is obsolete: true
Attachment #809521 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #63)
> (In reply to Mounir Lamouri (:mounir) from comment #62)
> > Inheriting from nsHTMLButtonFrame will pull a lot of quirks that exist in
> > <button> for historic reasons. It is not clear to me why we want to suffer
> > with that
> 
> What quirks are those?  Are you sure they're quirks of nsHTMLButtonFrame,
> and not <button>-specific?

I don't have an exhaustive list of them but there is a bug related to events not received by the inner part of the button, also a bug related to the inner focus ring making the button 1px too big. Boris would know more.
Flags: needinfo?(bzbarsky)
(In reply to Mounir Lamouri (:mounir) from comment #67)
> I don't have an exhaustive list of them but there is a bug related to events
> not received by the inner part of the button, also a bug related to the
> inner focus ring making the button 1px too big. Boris would know more.

After a quick try, inheriting from nsHTMLButtonFrame breaks the rendering of the element, and I wasn't able to fix this by modifying the css I had.
I suspect there are few things we don't want to inherit from, indeed.

The only problem I will have if I inherit from nsContainerFrame is that I will have to write a Reflow method. As bz stated in bug 664968 comment 5, it would be nice to have a container class with a default Reflow method.
This can be an interesting follow up bug.
> events not received by the inner part of the button

Not an issue here: the inner part is anon content which can't get events anyway from the pov of the page.

> a bug related to the inner focus ring making the button 1px too big.

This can be styled away by default.
Flags: needinfo?(bzbarsky)
Comment on attachment 809518 [details] [diff] [review]
Color input: layout part (6th)

>[mq]: color_layout

nit: still needs a commit message :)

>+nsresult
>+nsColorControlFrame::UpdateColor()
>+{
>+  // Get the color from the "value" property of our content; it will return the
>+  // default color (through the sanitization algorithm ) if there is none.

nit: drop the straggling space-before-paren -----------^

>+  // Set the background-color style property of the swatch element to this color
>+  return mColorContent->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
>+      NS_LITERAL_STRING(" background-color: ") + color, true);

Is there any reason we need that leading space before "background-color"? If we don't need it, let's drop it -- or if we need it for some arcane reason, add a comment briefly noting that.

>+++ b/layout/forms/nsColorControlFrame.h
>+  void AppendAnonymousContentTo(nsBaseContentList& aElements,
>+                                uint32_t aFilter) MOZ_OVERRIDE;
[...]
>+  bool IsLeaf() const MOZ_OVERRIDE { return true; }

Looks like you marked most of the MOZ_OVERRIDE methods as "virtual", but you missed these two. Please mark them as "virtual" as well.

>+  virtual bool IsFrameOfType(uint32_t aFlags) const MOZ_OVERRIDE
>+  {
>+    return nsBlockFrame::IsFrameOfType(aFlags &
>+      ~(nsIFrame::eReplaced | nsIFrame::eReplacedContainsBlock));
>+  }

[observation: once you inherit from nsHTMLButtonControlFrame (assuming we get that to work), you won't need this anymore, since this matches nsHTMLButtonControlFrame's impl]
[no action needed now on that; just noting it]

>+++ b/layout/reftests/forms/input/color/block-invalidate-1.html
>@@ -0,0 +1,15 @@
>+<!DOCTYPE html>
>+<html class='reftest-wait'>
>+  <link rel='stylesheet' type='text/css' href='style.css'>

Per comment 37, please remove the style.css lines from the testcases (the non "-ref" files). It's only used in the reference cases.

(Maybe it'd be worth naming it "reference-style.css", too, to make it clearer that it's providing style for reference cases to use in making mock buttons?

>diff --git a/layout/reftests/forms/input/color/reftest.list b/layout/reftests/forms/input/color/reftest.list
>+== input-color.html input-color-ref.html 
>+== margin-padding.html margin-padding-ref.html
>+== block-invalidate-1.html block-invalidate-1-ref.html
>+== transformations.html transformations-ref.html

Sorry -- I intended my review comment about renaming "block-invalidate" to apply to all of the reftests, not *just* to the block-invalidate files.  (I was just using that one as an example, since it's the first one in the patch, but I didn't make that clear enough -- sorry about that.)

So -- please add "-1" to the other tests (possibly easiest to do via search-and-replace in the patch file itself, as noted in comment 35).

With that, I think this looks good, aside from the changing-the-superclass stuff.
Attachment #809518 - Flags: review?(dholbert) → feedback+
Attachment #809521 - Flags: review?(dholbert) → feedback+
(In reply to Arnaud Bienner from comment #68)
> After a quick try, inheriting from nsHTMLButtonFrame breaks the rendering of
> the element, and I wasn't able to fix this by modifying the css I had.
> I suspect there are few things we don't want to inherit from, indeed.

I wouldn't give up so quickly. :) Given that the following renders just fine...
data:text/html,<button%20style="width:%2050px"><div%20style="height:%201em;width:100%;background:green">
...I'd posit that that's a proof-of-concept showing that we *can* render a nsHTMLButtonFrame with a piece of content inside of it that's just got a colorful background.

(I wonder if your broken rendering is due to bug 913759? It might be, if you have "height: 100%" on your color-swatch -- maybe try a fixed height & width (or fixed min-height & min-width), at least for now, to avoid problems like that, and I'll try to knock out that bug in the next few days, since I think we'll need that fixed for this to work.) 

> The only problem I will have if I inherit from nsContainerFrame is that I
> will have to write a Reflow method.

Right. (if we were to go that route)

> As bz stated in bug 664968 comment 5, it
> would be nice to have a container class with a default Reflow method.

It would -- though note that no one's done that probably because it's not obvious what this hypothetical container class's reflow method _should do_, particularly if it has multiple children (which isn't the case here, but could be the case in general, and is the case over in bug 664968). Should it place them side-by-side? top-to-bottom? Overlap them on top of each other, with everything at 0,0? It's unclear what the correct "generalized" reflow method would be.

Anyway, that's outside the scope of this bug. *If* we were implementing reflow for *this* frame (and that's a big "if", because I still think nsHTMLButtonControlFrame is the way to go), then we'd be guaranteed to just have 1 child, which would make the reflow method relatively easy.  (We'd basically just create a nsHTMLReflowState for the kid, and then call ReflowChild, and then FinishReflowChild, and then set our own "nsHTMLReflowMetrics" outparam's height & width based on whatever's in our own reflow state. It'd probably help to crib from some other relatively-recently-added frame classes like nsProgressFrame or nsRangeFrame.)
Depends on: 913759
> (Maybe it'd be worth naming it "reference-style.css", too, to make it
> clearer that it's providing style for reference cases to use in making mock
> buttons?

Indeed, good idea ;)

> Sorry -- I intended my review comment about renaming "block-invalidate" to
> apply to all of the reftests, not *just* to the block-invalidate files.  (I
> was just using that one as an example, since it's the first one in the
> patch, but I didn't make that clear enough -- sorry about that.)
> 
> So -- please add "-1" to the other tests (possibly easiest to do via
> search-and-replace in the patch file itself, as noted in comment 35).

I wasn't sure what you meant, that's why I changed only that one.
I'm not convinced it's a good idea: I named tests after the "use case" they should test.
By doing what you propose, I'm afraid we will end up with meaningless tests name like "test-1.html", "test-2.html".
I think there are very few cases where we will test the same things with only slight changes in two different tests. And even if this should happen, we might start numbering when adding these new tests.
What do you think?
(In reply to Arnaud Bienner from comment #72)
> By doing what you propose, I'm afraid we will end up with meaningless tests
> name like "test-1.html", "test-2.html".

I agree, "test-1.html" would indeed be a meaningless test name.

But "transformations-1.html", however, would not be meaningless. :) (or at least, it's as meaningful as "transformations.html")

Don't worry about having a directory full of reftests with "-1" suffixes -- that's fine. This is the convention we use for naming reftests in a relatively-future-proof way.

(It's not adhered to everywhere, but the not-adhering-to-it tests are mostly either old-and-sloppy, or a regression-test for a specific bug (and hence just named with the bug number), or they have already-absurdly-specific filenames-with-like-six-hyphenated-words.  None of those apply here.)

> I think there are very few cases where we will test the same things with
> only slight changes in two different tests.

This actually happens *all the time*. :)  See e.g. this directory of reftests that I wrote:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/flexbox/
and this directory of reftests that another guys is working on:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/margin-collapsing/

and e.g. just two days ago I posted a patch to add a new "-[n+1]" version of one of my tests in another bug.

It *might* not happen here, but we might as well plan to allow for it to happen.

> And even if this should happen,
> we might start numbering when adding these new tests.

It's better to name tests "correctly" up-front, in a future-proof way.

One reason is for test-failure tracking.  If one of these tests happens to intermittently fail on Windows XP (for example), and so someone files a bug named "transformations.html intermittently fails on WinXP", and then we rename the test to "transformations-1.html", then people trying to learn about subsequent intermittent failures (for the new name "transformations-1.html") won't discover the old intermittent-failure bug.  [We could rename the bug, but someone has to figure that out]

> What do you think?

I'd prefer you stick with the best-practice convention of using a "-1" suffix. :)
Attached patch Color input: layout part (7th) (obsolete) — — Splinter Review 
I've updated the patch regarding the last review.
I've also add another test, to test user-defined style.

nsColorControlFrame still inherit from nsBlockFrame in this patch.

I understand nsBlockFrame might not be the best choice, but at least it fits my need here:
- inheriting from nsContainerFrame means I will need to write a Reflow method: it sounds like reinventing the wheel, because nsBlockFrame::Reflow is OK for what I need here.
The reflow method we want sounded simple to me in the first place, but if we want to take into account all the CSS customization the user can do, I'm not sure it will as simple as described in comment 71.
In conclusion, I'm afraid of doing some duplicated work here.
One last thing "in favor" of nsBlockFrame is that the inner div (mColorContent->GetPrimaryFrame()) seems to be a nsBlockFrame object too.

- inheriting from nsHTMLButtonControlFrame doesn't sound like an easy solution, and I'm not sure it fits our requirements: in nsCSSFrameConstructor.cpp we need to use FCDATA_WITH_WRAPPING_BLOCK instead of SIMPLE_INT_CREATE to have it working. If we do so, setting the style attribute (to change the swatch background color) in nsColorControlFrame doesn't work anymore, which means we will need to change something else in the core layout engine (something like changes D. Baron already reviewed I guess).
In addition of this extra work, I think we will inherit some pseudo selectors we don't really need here, which is useless and probably not very efficient. I'm not sure how "non efficient" using nsBlockFrame is in comparison.

I already spent lot of time working in this bug but now I'm quite busy and I don't have lot of time to work on this atm.
I would be in favor of having my patch landed as it is now, and having someone more experienced working on this (or finishing the patch if it cannot be landed as it is).
Attachment #809518 - Attachment is obsolete: true
Attachment #809521 - Attachment is obsolete: true
Attachment #816821 - Flags: review?(dholbert)
The inter-diff.
(In reply to Arnaud Bienner from comment #74)
> I already spent lot of time working in this bug but now I'm quite busy and I
> don't have lot of time to work on this atm.
> I would be in favor of having my patch landed as it is now, and having
> someone more experienced working on this (or finishing the patch if it
> cannot be landed as it is).

Something like that should be fine, yeah.  (BTW, big thanks for all your work on this bug & the other <input type="color"> bugs!)

I'll take care of this review tomorrow.
Comment on attachment 816821 [details] [diff] [review]
Color input: layout part (7th)

Looks like this fixes everything from comment 70, with one exception:

>+  // Set the background-color style property of the swatch element to this color
>+  return mColorContent->SetAttr(kNameSpaceID_None, nsGkAtoms::style,
>+      NS_LITERAL_STRING(" background-color:") + color, true);

Drop the space character at the beginning of this quoted string; it looks arbitrary, and I don't think it serves any purpose.

(I mentioned this in comment 70, and I think you dropped a different space character -- the one at the end of this quoted string (which was also drop-worthy, so that's good.)) :)

r=me with that. (and with the assumption that someone (possibly me) will tweak this to inherit from nsHTMLButtonFrame)

(Over the next few days, I'll experiment with the nsHTMLButtonFrame inheritance, in a patch that layers on top of this one. Until I've done that experimenting, I'd like to hold off on landing this.)
Attachment #816821 - Flags: review?(dholbert) → review+
Comment on attachment 816821 [details] [diff] [review]
Color input: layout part (7th)

One other nit, in contextual code, while you're fixing that extra space:

>+nsresult
>+nsColorControlFrame::UpdateColor()
>+{
>+  // Get the color from the "value" property of our content; it will return the
>+  // default color (through the sanitization algorithm) if there is none.
>+  nsAutoString color;
>+  nsCOMPtr<nsIDOMHTMLInputElement> elt = do_QueryInterface(mContent);
>+  elt->GetValue(color);
>+
>+  // Set the background-color style property of the swatch element to this color

We should add an assertion to validate that the comment is correct -- in particular, we should assert that |color| is nonempty here.

i.e. add something like this after the GetValue() line:
  MOZ_ASSERT(!color.IsEmpty(),
             "Content node's GetValue() should return a valid color string "
             "(the default color, if no valid color is set)");

(If you don't get to this, I'm happy to have this be part of the followup patch, too.)
Attached patch Color input: layout part (8th) (obsolete) — — Splinter Review 
New patch with your comments applied. I've also removed some useless trailing spaces I noticed.
I've put it directly in "r+" because there was only few minor changes to do.
I've added "r=dbaron,dholbert" at the end of the patch name (even if dbaron only reviewed part of this patch). Let me know if this is not OK for you.
Attachment #816821 - Attachment is obsolete: true
Attachment #816824 - Attachment is obsolete: true
Attachment #818078 - Flags: review+
Corresponding interdiff.
Thanks for fixing that up.

Posting an updated version, rebased to apply on top of current mozilla-central tip, and with the reftest reference cases' names fixed. (When testing, I noticed that some of them were named like "custom-style-ref-1.html", with "ref-1" at the end, which is backwards -- our convention is to put "ref" at the very end. The idea is to make it easy to guess the reference name from the test name by always just adding "-ref" at the very end, before the file-extension. Anyway, I just fixed that while rebasing, rather than bothering you with it.)
Attachment #818256 - Flags: review+
Attachment #818079 - Attachment is obsolete: true
Attachment #818078 - Attachment is obsolete: true
OK, I've got a patch ready to make this inherit from nsHTMLButtonControlFrame, coming up.
Attached patch part 2: simplify forms.css (obsolete) — — Splinter Review 
This simplifies the forms.css chunk from part 1, in preparation for inheriting from nsHTMLButtonFrame.

In particular:
 - We don't need the "input[type="color"] > div" selector, on top of the pseudo-element selector.
 - I don't think we benefit from setting "border-width" to 0. (There won't be a border, due to default "border-style: none"; and if someone changes that, they probably want to see the border so we shouldn't set it to 0 out from under them.)
 - We don't benefit from "margin-left"/"margin-right" (for horizontal centering).  nsHTMLButtonControlFrame::Reflow already has logic to center its contents.
 - We don't benefit from "text-align: center" because we don't have any text (and there's no direct way for authors to get text into the swatch, either, IIUC. And if we end up adding/finding a way, then those authors can center the text if they like.)

This also adds a TODO comment to remind us to share the other buttons' style with input[type="color"] once we're getting ready to pref it on by default. (but probably not before, per comment 8 here.)
Attachment #818791 - Flags: review?(arnaud.bienner)
This patch:
 - Adds a chunk to ElementForStyleContext() to help us find the <input> element when handling dynamic restyling of the color swatch frame.*
 - Changes the nsCSSFrameConstructor boilerplate to match what we do for other button-based frames.
 - Changes the superclass to be nsHTMLButtonControlFrame (which I've typedeffed to "nsColorControlFrameSuper", following a pattern used in some other frame classes)
 - Adds one missing MOZ_OVERRIDE
 - Removes two methods (BuildDisplayList, IsFrameOfType) that we no longer need because we inherit versions that work from nsHTMLButtonControlFrame.

*(Arnaud: That first chunk is I think what you were missing, when you tried this & were having trouble with this frame snapping to be tiny-sized every time you tweaked its color. It took me a bit of time to realize what was wrong, but the corresponding ElementForStyleContext chunk in bug 635240's patch helped me realize that we need this.

Basically, without this ElementForStyleContext chunk, we end up ignoring your forms.css rule after restyles (and hence we don't have any specified width/min-width/etc and we snap to 0 size), because our parent is an anonymous div, which makes us look for rules like "div::-moz-color-swatch", and we find nothing. The ElementForStyleContext tweak makes us use our grandparent, the <input> frame, for pseudoElement rule lookups; that means we'll look for rules that look like "input::-moz-color-swatch", which lets us keep honoring the forms.css rule.)
Attachment #818801 - Flags: review?(jwatt)
Attachment #818801 - Flags: feedback?(arnaud.bienner)
This tweaks the reftests to account for the fact that they're button-based.

(The old reftests don't quite pass, due to what look like baseline-alignment behavior differences between <button>-derived things vs. nsBlockFrame-styled-with-"display:inline-block"-derived things. IIRC, the baseline of an inline-block is the bottom of the block, whereas the baseline of a button is the baseline of its contents... or something like that.)

Anyway; the actual changes are:
 - Removed reference-style.css from block-invalidate-1-ref.html, where it's unused. (That reference case has an actual <input type="color"> rather than a fake one, so it doesn't need the fake styling.)

 - Replaced <div class="input-color"> with <button class="input-color"> in reference cases (& s/div/button/ in selectors, etc.)

 - Removed some unused declarations from reference-style.css (e.g. "text-shadow: none;", etc)

 - Added a ::-moz-focus-inner selector to reference-style.css, to zero out some "bonus padding" that gets applied to buttons on some platforms.

With these changes, the reftests pass on my machine.
Attachment #818810 - Flags: review?(arnaud.bienner)
Attachment #818801 - Flags: feedback?(arnaud.bienner) → feedback+
Comment on attachment 818801 [details] [diff] [review]
part 3: inherit from nsHTMLButtonControlFrame

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

::: layout/forms/nsColorControlFrame.h
@@ +5,5 @@
>  
>  #ifndef nsColorControlFrame_h___
>  #define nsColorControlFrame_h___
>  
> +#include "nsHTMLButtonControlFrame.h"

It's probably better to keep include in alphabetical order IMHO. i.e. move this line after #include "nsCOMPtr.h"
(In reply to Arnaud Bienner from comment #86)
> It's probably better to keep include in alphabetical order IMHO. i.e. move
> this line after #include "nsCOMPtr.h"

Fixed. Thanks!
Attachment #819053 - Flags: review?(jwatt)
Attachment #819053 - Flags: feedback+
(In reply to Daniel Holbert [:dholbert] from comment #83)
>  - We don't need the "input[type="color"] > div" selector, on top of the
> pseudo-element selector.

Yes indeed: I added it at the beginning, before implementing the pseudo element. Not needed anymore.

>  - We don't benefit from "margin-left"/"margin-right" (for horizontal
> centering).  nsHTMLButtonControlFrame::Reflow already has logic to center
> its contents.

Trying to style size of the input and the swatch (inner div), it looks like the inner div is vertically centered, but not horizontally, e.g.: 
  input[type=color] {
    height:50px;
    width:100px;
  }
  input[type=color]::-moz-color-swatch {
    height:30px;
    width:80px;
  }

That's why I added the auto margin.
IMHO it's nicer and we should keep this.
Oh, you're right; buttons get their vertical centering from their custom reflow code, but their horizontal centering from "text-align: center". So there's no horizontal centering reflow magic like I thought there was.

So, you're right -- the auto margins make sense.  Here's an updated patch that doesn't remove them.
Attachment #818791 - Attachment is obsolete: true
Attachment #818791 - Flags: review?(arnaud.bienner)
Attachment #819409 - Flags: review?(arnaud.bienner)
Attachment #818801 - Attachment is obsolete: true
Attachment #818801 - Flags: review?(jwatt)
Attachment #819409 - Flags: review?(arnaud.bienner) → review+
Attachment #818810 - Flags: review?(arnaud.bienner) → review+
So for a somewhat-arcane reason, two reftests actually fail on Windows, with or without my followup patches.

The somewhat-arcane reason is that the "padding" declaration in reference-style.css has a *different effect* from the "padding" declaration in forms.css (because reference-style.css is an author stylesheet, whereas forms.css is a UA stylesheet; and on Windows, we make a distinction between the two when deciding to add some theme-specific widget padding). (See the long comment in the attached patch for more info.)

This patch adds a "testcase-style.css" file, which now will be used in all testcases where we use reftest-style.css, which sets "padding" and fixes the problem.

(Once we are ready to turn this on by default and can actually add input[type="color"] styling in forms.css, we'll be able to make sure it has the same padding as <button>, which will mean we can drop testcase-style.css and reference-style.css (or at least its "padding" line that's causing this rendering difference).)

Here's a windows-specific Try run, with just Arnaud's patch, showing the test failure:
 https://tbpl.mozilla.org/?tree=Try&rev=85a00cd566b7
and here's a broader Try run with all of the followups applied (including this one), with the tests passing:
 https://tbpl.mozilla.org/?tree=Try&rev=85a00cd566b7

(As a tangential additional tweak: this patch removes two pieces of un-exercised custom styling ("cursor" and "overflow") from the custom-style* files -- those styles don't affect the test's rendering, since there's no cursor rendered in reftests and the contents don't overflow.)
Attachment #819655 - Flags: review?(arnaud.bienner)
(In reply to Daniel Holbert [:dholbert] from comment #90)
> and here's a broader Try run with all of the followups applied (including
> this one), with the tests passing:

Sorry, the correct URL for the Try run w/ fixes is https://tbpl.mozilla.org/?tree=Try&rev=3bf21191ab6f
Comment on attachment 819655 [details] [diff] [review]
part 5: Fix reftest failure on windows by specifying "padding" in testcases

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

::: layout/reftests/forms/input/color/custom-style-1-ref.html
@@ -4,5 @@
>    <style>
>      button.input-color {
>        -moz-appearance: button;
> -      cursor: default;
> -      overflow:hidden;

Why removing these lines?
We don't need them to have the test succeed, but this was an idea about what can be the default style we may have for input type color in form.css.
I'm not sure about "overflow:hidden", but "cursor: default" should be added IMO.
(In reply to Arnaud Bienner from comment #92)
> Why removing these lines?
> We don't need them to have the test succeed, but this was an idea about what
> can be the default style we may have for input type color in form.css.
> I'm not sure about "overflow:hidden", but "cursor: default" should be added
> IMO.

See the last paragraph in comment 90. Basically, these lines aren't useful in the reftest, since the reftest doesn't exercise them at all.
Blocks: 928891
BTW, I filed bug 928891 on adding the forms.css styling. We'll get the "cursor: default" styling that you're concerned about once that bug is resolved (from sharing other button styles).
Also: the B2G R6 test failures shown in comment 91's Try run are interesting -- the reftests are failing there because we apparently don't honor "-moz-appearance: textfield" on B2G. (B2G insists on rendering the button like a button, ignoring "-moz-appearance".) I filed bug 928877 on this, and until that's fixed I think we can simply mark the tests as "fails-if(B2G)" with a reference to that bug.
(Here's a patch that adds the "fails-if(B2G)" annotations, per comment 95. block-invalidate-1.html is the only one that doesn't need the annotation, since its reference case uses <input type="color"> and hence renders with the same theming as the testcase.

I don't think this part needs review since it's just tweaking a manifest to annotate some known failures.)
Comment on attachment 819693 [details] [diff] [review]
part 6: annotate tests as failing on B2G due to bug 928877

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

::: layout/reftests/forms/input/color/reftest.list
@@ +5,3 @@
>  == block-invalidate-1.html block-invalidate-1-ref.html
> +fails-if(B2G) == transformations-1.html transformations-1-ref.html # bug 928877
> +fails-if(B2G) == custom-style-1.html custom-style-1-ref.html # bug 928877

Just a remark: I saw this one failed, but I'm surprised as we apply custom styling  here ("-moz-appearance:button") so it should look like a button :( Or does B2G also automatically adds padding, etc. in addition of -moz-appearance?
(In reply to Arnaud Bienner from comment #97)
I'm not sure I understand what you're saying there.

> but I'm surprised as we apply custom styling  here ("-moz-appearance:button")

We only apply "-moz-appearance: button" in one test/reference -- custom-style-1* -- and that's the one that passes on B2G.

> does B2G also automatically adds padding, etc

I don't believe it does, but that's unrelated. As far as I know, the B2G failure is purely related to the gradient shading (whether it's inset like a textbox, or outset like a button) -- not padding/etc.
I'd like to base a patch in bug 922669 on your part 1 changes (the bits that pass the anonymous content node into PseudoElementRuleProcessorData).  Daniel/Arnaud, is it OK to land that patch before the rest?
Flags: needinfo?(arnaud.bienner)
Attachment #819655 - Flags: review?(arnaud.bienner) → review+
Nope, "part 1" has reftest failures on Windows, as shown in the Try run in comment 90, so it can't land on its own. (The reftests are fixed in part 5 here, though.)

Let's just hammer on jwatt to review part 3, so that we can land the whole stack at once. ;)
Flags: needinfo?(arnaud.bienner)
Flags: needinfo?(jwatt)
Comment on attachment 819053 [details] [diff] [review]
part 3 v2: inherit from nsHTMLButtonControlFrame [feedback=arnaud.bienner]

Ok, looks good.
Attachment #819053 - Flags: review?(jwatt) → review+
Thanks, jwatt! Here's one final sanity-check Try run: https://tbpl.mozilla.org/?tree=Try&rev=bb503c268dd3

If that looks good, I'll push later today.
Flags: needinfo?(jwatt)
Blocks: 930277
Depends on: 977029
Depends on: 1167782
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: