283219 - trigger with appearance minimal scenario fails

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[aaronr]

Testcase submitted by a XForms user of a common XForms scenario used by XForms
developers combining @appearance="minimal" on a trigger with CSS underlining
style to create a trigger that looks just like a HTML link.  Currently we just
show a pushbutton since we haven't implemented @appearance="minimal" on trigger
elements, yet.

Comment 1

[aaronr]

Attachment: testcase

testcase that demonstrates the problem

Comment 2

[Allan Beaufour]

Attachment: Sketch

This patch creates a <span> when @appearance="minimal", but DOMActivate does
not work. What am I doing wrong?

Styling is not working either, should I copy the style attribute from the
<trigger> to the span?

Comment 3

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

(In reply to comment #2)
> Styling is not working either, should I copy the style attribute from the
> <trigger> to the span?

Style attribute is defined only for (X)HTML, XUL and SVG elements,
so I'd say the test case is not valid.

This doesn't mention style attribute:
"A host language is expected to add attributes such as xml:lang as well 
as an attribute, named class, that holds a list of strings that can be 
matched by CSS class selectors."

Comment 4

[Leigh L. Klotz, Jr.]

Attachment: trigger/@minimal using class instead of style

Here is an updated test case using XHTML 1 strict (style and @class) instead of
@style.  The issue of whether the host language provides @style on XForms
controls is one of whether your host language is XHTML transitional or XHTML
strict, and is unrelated to this functionality.

Comment 5

[Leigh L. Klotz, Jr.]

Attachment: Proper placement of @class on trigger instead of on label

Comment 6

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

We should/could extend XTF so that we can specify the name of the class attribute.
So something like 
nsIAtom nsIXTFElement::getClassAttributeName() which would then internally
be actually nsIStyledContent::GetClassAttributeName(). 
I'll open a new bug for this.

Comment 7

[Allan Beaufour]

Attachment: Using UIEvent

DOMActivate is UI Event, so this patch creates such a one instead... but it
still does not work :(

Comment 8

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Maybe we could just (un)style the button.
Adding something like this to the xforms.css (didn't test):

@namespace xf url(http://www.w3.org/2002/xforms);
@namespace html url(http://www.w3.org/1999/xhtml);
xf|button[appearance=minimal] > html|button {
  background: transparent;
  border:none;
  color: inherit;
  margin: 0px;
  padding: 0px;
}

Comment 9

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Comment on attachment 175340 [details]
Proper placement of @class on trigger instead of on label

you could write valid CSS ;)

Comment 10

[Leigh L. Klotz, Jr.]

Attachment: Remove extraneous text from CSS to correct syntax

Please accept my apologies for carelessness in cut and paste.

Comment 11

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Attachment: alternative patch

This makes minimal trigger and submit *look* more like a normal text, which
can then be styled to look like a link.
For class attribute the patch in Bug 283366 is needed.

Comment 12

[Allan Beaufour]

(In reply to comment #11)
> Created an attachment (id=175474) [edit]
> alternative patch
> 
> This makes minimal trigger and submit *look* more like a normal text, which
> can then be styled to look like a link.
> For class attribute the patch in Bug 283366 is needed.

Hmmm, it's fine to do it that way as we can do what we want. But using a span
f.x. allows one to get the minimal trigger word wrapped, which I personally
think is nicer.

Comment 13

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Dispatching UIEvents should work now. (Bug 283637)

Comment 14

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Comment on attachment 175422 [details] [diff] [review]
Using UIEvent

DOMActivate works now with this patch, but dynamic changes to the appearance
attribute don't do anything.

Comment 15

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Attachment: based on attachment 175422 , supports dynamic changes

Comment 16

[Allan Beaufour]

Comment on attachment 175755 [details] [diff] [review]
based on attachment 175422 [details] [diff] [review] , supports dynamic changes

r=me

Comment 17

[Doron Rosenberg (IBM)]

Comment on attachment 175755 [details] [diff] [review]
based on attachment 175422 [details] [diff] [review] , supports dynamic changes

>+  if (aName == nsXFormsAtoms::appearance) {
>+    if (mIsMinimal != aValue.EqualsLiteral("minimal")) {
>+      nsCOMPtr<nsIDOMNode> parent;
>+      nsCOMPtr<nsIDOMNode> next;
>+      nsCOMPtr<nsIDOMNode> tmp;

can't you define them all on one line?

r=doron

Comment 18

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Attachment: attachment 175755 + comments

Comment 19

[Doron Rosenberg (IBM)]

checked in.

Comment 20

[Olli Pettay [:smaug][[e-mail]] (holiday -> Aug30)]

Attachment: s/WillSetAttribute/AttributeSet/

oops.

Comment 21

[Allan Beaufour]

(In reply to comment #20)
> Created an attachment (id=176037) [edit]
> s/WillSetAttribute/AttributeSet/
> 
> oops.

Checked it in.