The default bug view has changed. See this FAQ.

Add a pseudo-element to access <progress> bar element.

RESOLVED FIXED in mozilla6

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

({dev-doc-complete, html5})

Trunk
mozilla6
dev-doc-complete, html5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
Something like:
progress::-moz-progress-bar
For reference: https://svn.webkit.org/wiki/Styling%20Form%20Controls
(Assignee)

Comment 2

6 years ago
(In reply to comment #1)
> For reference: https://svn.webkit.org/wiki/Styling%20Form%20Controls

FWIW, I don't like the name they chose.
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
Created attachment 512282 [details] [diff] [review]
Patch v1

Actually, the tests cases showed an edge-case issue. Looks like they are useful ;)
Attachment #512126 - Attachment is obsolete: true
Attachment #512282 - Flags: review?(roc)
(Assignee)

Comment 6

6 years ago
Created attachment 512283 [details] [diff] [review]
Patch v1

I-just-realized-I-forgot-something-when-I-pressed-Submit syndrome :(
Attachment #512282 - Attachment is obsolete: true
Attachment #512283 - Flags: review?(roc)
Attachment #512282 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Whiteboard: [needs review]
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.
Attachment #512283 - Flags: review?(roc) → review?(dbaron)
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.
Attachment #512283 - Flags: review?(dbaron) → review-
(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.
(Assignee)

Comment 10

6 years ago
(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?
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?
(Assignee)

Comment 12

6 years ago
(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.
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...
(Assignee)

Comment 14

6 years ago
(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.
Fwiw, I would be happy to write a patch to that effect if you want.
(Assignee)

Comment 16

6 years ago
Created attachment 530185 [details] [diff] [review]
Inter diff
(Assignee)

Comment 17

6 years ago
Created attachment 530186 [details] [diff] [review]
Patch v1.1
Attachment #512283 - Attachment is obsolete: true
Attachment #530186 - Flags: review?(dbaron)
(Assignee)

Comment 18

6 years ago
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).
Attachment #530186 - Attachment is obsolete: true
Attachment #530284 - Flags: review?(dbaron)
Attachment #530186 - Flags: review?(dbaron)
(Assignee)

Updated

6 years ago
Depends on: 654990
(Assignee)

Comment 19

6 years ago
(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 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.
Attachment #530284 - Flags: review?(dbaron) → review+
(Assignee)

Comment 21

6 years ago
(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.
Whiteboard: [needs review] → [ready to land][waits for dependencies]
(Assignee)

Comment 22

6 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/2139b047dbb7
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [ready to land][waits for dependencies]
Target Milestone: --- → mozilla6

Updated

6 years ago
Depends on: 655860
Backed out in http://hg.mozilla.org/mozilla-central/rev/dd9ba28d2bd9 to resolve bug 655860.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 24

6 years ago
The regression wasn't caused by these patches. Re-landed:
http://hg.mozilla.org/mozilla-central/rev/dd09b7ba02ff
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
No longer depends on: 655860
Is the point of this to be able to access the currently-filled part of the progress bar within the full progress bar?
(Assignee)

Comment 26

6 years ago
(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.
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.