Last Comment Bug 633209 - Add a pseudo-element to access <progress> bar element.
: Add a pseudo-element to access <progress> bar element.
Status: RESOLVED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Mounir Lamouri (:mounir)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 654990
Blocks: 633207
  Show dependency treegraph
 
Reported: 2011-02-10 08:21 PST by Mounir Lamouri (:mounir)
Modified: 2011-06-16 13:56 PDT (History)
8 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (10.77 KB, patch)
2011-02-14 02:02 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1 (17.62 KB, patch)
2011-02-14 14:25 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1 (17.86 KB, patch)
2011-02-14 14:27 PST, Mounir Lamouri (:mounir)
dbaron: review-
Details | Diff | Splinter Review
Inter diff (1.75 KB, patch)
2011-05-04 15:51 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.1 (16.16 KB, patch)
2011-05-04 15:52 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.2 (22.16 KB, patch)
2011-05-05 05:32 PDT, Mounir Lamouri (:mounir)
dbaron: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-02-10 08:21:17 PST
Something like:
progress::-moz-progress-bar
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2011-02-11 02:01:23 PST
For reference: https://svn.webkit.org/wiki/Styling%20Form%20Controls
Comment 2 Mounir Lamouri (:mounir) 2011-02-11 02:16:46 PST
(In reply to comment #1)
> For reference: https://svn.webkit.org/wiki/Styling%20Form%20Controls

FWIW, I don't like the name they chose.
Comment 3 Mounir Lamouri (:mounir) 2011-02-14 02:00:24 PST
I haven't been able to do anything else than setting the border and the background-color with webkit's pseudo-element. I think we should let the authors manipulate the bar like any element.
Comment 4 Mounir Lamouri (:mounir) 2011-02-14 02:02:42 PST
Created attachment 512126 [details] [diff] [review]
WIP

Actually, it's not really WIP but I didn't test the patch against the test suite I wrote (not on the same computer) but it should be fine.
Comment 5 Mounir Lamouri (:mounir) 2011-02-14 14:25:07 PST
Created attachment 512282 [details] [diff] [review]
Patch v1

Actually, the tests cases showed an edge-case issue. Looks like they are useful ;)
Comment 6 Mounir Lamouri (:mounir) 2011-02-14 14:27:47 PST
Created attachment 512283 [details] [diff] [review]
Patch v1

I-just-realized-I-forgot-something-when-I-pressed-Submit syndrome :(
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-02-28 14:53:16 PST
Comment on attachment 512283 [details] [diff] [review]
Patch v1

I think you need to do something to ensure that the pseudo-element style context is set up correctly when we restyle the frame ... but I'm not sure what.
Comment 8 David Baron :dbaron: ⌚️UTC-10 2011-04-25 18:30:08 PDT
Comment on attachment 512283 [details] [diff] [review]
Patch v1

Given what you do in nsProgressFrame::SetInitialChildList, you need
to enforce a few invariants that would have caused us to construct a
different sort of frame.  In particular, forms.css needs to have:

::-moz-progress-bar {
  /* block styles that would change the type of frame we construct */
  display: block ! important;
  float: none ! important;
  position: static ! important;
  overflow: visible ! important;
}

And while I'm on the topic, I don't see any reason the selector shouldn't
change to what I wrote above (rather than progress::-moz-progress-bar.


That said, rather than overriding SetInitialChildList the way you do, I
think you should implement nsIAnonymousContentCreator::CreateFrameFor to
create the frame with the correct style context initially.

>   // We want the frame to take all the available size.
>-  reflowState.SetComputedWidth(availSize.width);
>+  nscoord width = availSize.width - reflowState.mComputedMargin.LeftRight()
>+                                  - reflowState.mComputedBorderPadding.LeftRight();
>+  width = NS_MAX(width, 0);
>+  reflowState.SetComputedWidth(width);

Instead of doing this, why not just stick width: auto ! important in
forms.css ?  It seems like that's basically what you're doing here.


But not using the same nsHTMLReflowMetrics for two different frames
certainly looks good.  That's a problem introduced in bug 567872's
patch, though.

>+  if (GetStyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL) {
>+    xoffset += reflowState.mComputedMargin.right;
>+  } else {
>+    xoffset += reflowState.mComputedMargin.left;
>   }

This is wrong; you should just add the left margin, since you're
determining the top-left corner of the frame.


I believe restyling the frame should just work.

>+  newStyleContext = barFrame->PresContext()->PresShell()->StyleSet()->

At least omit the "->PresShell()", since PresContext() has a StyleSet().



Sorry for the huge delay getting to this (and bug 567872).  I should be able to look at a revised patch much faster.
Comment 9 David Baron :dbaron: ⌚️UTC-10 2011-04-25 18:44:33 PDT
(In reply to comment #8)
> >   // We want the frame to take all the available size.
> >-  reflowState.SetComputedWidth(availSize.width);
> >+  nscoord width = availSize.width - reflowState.mComputedMargin.LeftRight()
> >+                                  - reflowState.mComputedBorderPadding.LeftRight();
> >+  width = NS_MAX(width, 0);
> >+  reflowState.SetComputedWidth(width);
> 
> Instead of doing this, why not just stick width: auto ! important in
> forms.css ?  It seems like that's basically what you're doing here.

Er, ignore this comment, mostly.  Except I think you actually should be passing something different to the reflow state constructor, and only using this calculation for the SetComputedWidth call.  But I'll deal with that in bug 567872.
Comment 10 Mounir Lamouri (:mounir) 2011-05-04 10:18:25 PDT
(In reply to comment #8)
> That said, rather than overriding SetInitialChildList the way you do, I
> think you should implement nsIAnonymousContentCreator::CreateFrameFor to
> create the frame with the correct style context initially.

If I understand correctly, I will have to define a nsCSSFrameConstructor::ConstructProgressFrame method if I use CreateFrameFor. Does it worth adding this complexity?
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2011-05-04 10:48:24 PDT
You shouldn't need a separate ctor method.  Just need to make sure your frame is an nsIAnonymousContentCreator and not a leaf.  Or is it a leaf?
Comment 12 Mounir Lamouri (:mounir) 2011-05-04 10:54:32 PDT
(In reply to comment #11)
> You shouldn't need a separate ctor method.  Just need to make sure your frame
> is an nsIAnonymousContentCreator and not a leaf.  Or is it a leaf?

It's a nsIAnonymousContentCreator and it's not a leaf AFAIUI (it has a child).

Though, I get these asserts:
###!!! ASSERTION: If you need to use CreateFrameFor, you need to call CreateAnonymousFrames manually and not follow the standard ProcessChildren() codepath for this frame: '!creator || !creator->CreateFrameFor(anonymousItems[i])', file /home/volkmar/projects/mozilla/html5forms/layout/base/nsCSSFrameConstructor.cpp, line 9546
###!!! ASSERTION: asked to create frame construction item for a node that already has a frame: 'Error', file /home/volkmar/projects/mozilla/html5forms/layout/base/nsCSSFrameConstructor.cpp, line 4998

There seem to be two consumers of CreateFrameFor and both have a ctor method in nsCSSFrameConstructor calling CreateAnonymousFrames.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2011-05-04 11:02:52 PDT
Oh, you need to use CreateFrameFor?  In that case, yeah, that would involve having a ctor function...

But you don't really want to create your own frame, right?  Just to hand out a style context?  We've talked about changing nsIAnonymousContentCreator to hand out style contexts, not just content nodes, before...
Comment 14 Mounir Lamouri (:mounir) 2011-05-04 11:08:52 PDT
(In reply to comment #13)
> Oh, you need to use CreateFrameFor?  In that case, yeah, that would involve
> having a ctor function...

I don't know if I *need*. David has recommended that approach on comment 8.

> But you don't really want to create your own frame, right?  Just to hand out a
> style context?  We've talked about changing nsIAnonymousContentCreator to hand
> out style contexts, not just content nodes, before...

I guess so, yes.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-05-04 11:13:53 PDT
Fwiw, I would be happy to write a patch to that effect if you want.
Comment 16 Mounir Lamouri (:mounir) 2011-05-04 15:51:36 PDT
Created attachment 530185 [details] [diff] [review]
Inter diff
Comment 17 Mounir Lamouri (:mounir) 2011-05-04 15:52:05 PDT
Created attachment 530186 [details] [diff] [review]
Patch v1.1
Comment 18 Mounir Lamouri (:mounir) 2011-05-05 05:32:14 PDT
Created attachment 530284 [details] [diff] [review]
Patch v1.2

Add RTL tests and change display: block !important to display: inline-block !important (tests were failing).
Comment 19 Mounir Lamouri (:mounir) 2011-05-05 06:28:06 PDT
(In reply to comment #15)
> Fwiw, I would be happy to write a patch to that effect if you want.

I've open bug 654989.
Comment 20 David Baron :dbaron: ⌚️UTC-10 2011-05-05 16:09:39 PDT
Comment on attachment 530284 [details] [diff] [review]
Patch v1.2

r=dbaron, but please file a followup on fixing this SetInitialChildList stuff to use the api added in bug 654989.
Comment 21 Mounir Lamouri (:mounir) 2011-05-06 02:54:08 PDT
(In reply to comment #20)
> Comment on attachment 530284 [details] [diff] [review] [review]
> Patch v1.2
> 
> r=dbaron, but please file a followup on fixing this SetInitialChildList
> stuff to use the api added in bug 654989.

I've open bug 654990 for that.
Comment 22 Mounir Lamouri (:mounir) 2011-05-09 05:37:05 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/2139b047dbb7
Comment 23 Shawn Wilsher :sdwilsh 2011-05-09 16:11:16 PDT
Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.
Comment 24 Mounir Lamouri (:mounir) 2011-05-10 06:56:41 PDT
The regression wasn't caused by these patches. Re-landed:
http://hg.mozilla.org/mozilla-central/rev/dd09b7ba02ff
Comment 25 Eric Shepherd [:sheppy] 2011-05-31 14:07:57 PDT
Is the point of this to be able to access the currently-filled part of the progress bar within the full progress bar?
Comment 26 Mounir Lamouri (:mounir) 2011-05-31 14:50:18 PDT
(In reply to comment #25)
> Is the point of this to be able to access the currently-filled part of the
> progress bar within the full progress bar?

Yes, it will let the author style the bar inside the progress bar.
Comment 27 Eric Shepherd [:sheppy] 2011-06-16 13:56:19 PDT
Documented:

https://developer.mozilla.org/en/CSS/::-moz-progress-bar

Also updated:

https://developer.mozilla.org/en/HTML/Element/progress
and Firefox 6 for developers.

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