Closed Bug 321311 Opened 20 years ago Closed 10 years ago

range for xul

Categories

(Core Graveyard :: XForms, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: surkov, Unassigned)

References

Details

Attachments

(4 files, 9 obsolete files)

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1 It's needed to realize the xforms:range element for xul. Reproducible: Always
I guess range for xul should be based on new proposed widget slider (see the bug 290255). The advantages are users get behaviour and look'n'feel of range element as he expect.
Depends on: 290255
Blocks: 327236
Severity: normal → enhancement
Assignee: aaronr → surkov
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 343525
Attached file testcase
Blocks: 348439
Attached patch patch (obsolete) — Splinter Review
Attachment #233363 - Flags: review?(Olli.Pettay)
Status: NEW → ASSIGNED
just a thought...you might want to sync this with msterlin's work on the xhtml range for determining defaults for attributes that don't exist (bug 331987)...to keep as much code common as possible.
(In reply to comment #4) > just a thought...you might want to sync this with msterlin's work on the xhtml > range for determining defaults for attributes that don't exist (bug > 331987)...to keep as much code common as possible. > Yes and yes. I know about it and I'd like it :). Methods from msterlin's patch should be hosted in common binding for ranges that my patch adds. Merle, do you like to wait for this patch? It helps us to avoid doing one and the same work twice since if your patch will be checked in as it is then later we should remove your methods from range for xhtml binding and add them to common range binding.
after you merge msterlin's patches and your patches, please verify that the testcases attached to bug 331987 still pass. Thanks.
Comment on attachment 233363 [details] [diff] [review] patch Waiting for bug 348439.
Attachment #233363 - Flags: review?(Olli.Pettay)
No longer blocks: 348439
Depends on: 348439
Depends on: 352462
No longer depends on: 348439
Blocks: 334603
Attached patch patch2 (obsolete) — Splinter Review
Attachment #233363 - Attachment is obsolete: true
Attachment #240177 - Flags: review?(Olli.Pettay)
Attached file testcase2
The problem is initial refresh of range is not called if it contains xforms label. I can't get the mystics but I think the things in patch go right.
With the patch I get this error: WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed: file /home/smaug/mozilla/mozilla_cvs/mozilla/content/xbl/src/nsXBLService.cpp, line 1207 XML Parsing Error: no element found Location: jar:file:///home/smaug/mozilla/mozilla_cvs/mozilla/ff_build/dist/bin/extensions/%7Bcf2812dc-6a7c-4402-b639-4d277dac4c36%7D/chrome/xforms.jar!/content/xforms/range-xul.xml Line Number 1, Column 1:
Comment on attachment 240177 [details] [diff] [review] patch2 So this patch clearly doesn't work ;)
Attachment #240177 - Flags: review?(Olli.Pettay) → review-
(In reply to comment #10) > XML Parsing Error: no element found > Location: > jar:file:///home/smaug/mozilla/mozilla_cvs/mozilla/ff_build/dist/bin/extensions/%7Bcf2812dc-6a7c-4402-b639-4d277dac4c36%7D/chrome/xforms.jar!/content/xforms/range-xul.xml > Line Number 1, Column 1: I've seen the same 'no element found' error developing the bindings for the non-numeric range types and in my case each time it was because I forgot to add the new binding to jar.mn. I don't see any changes to jar.mn in this patch, so maybe that is a clue.
Attached patch patch3 (obsolete) — Splinter Review
Attachment #240177 - Attachment is obsolete: true
Attachment #240180 - Flags: review?(Olli.Pettay)
(In reply to comment #9) > The problem is initial refresh of range is not called if it contains xforms > label. I can't get the mystics but I think the things in patch go right. > I'd like know the reason for this and get the problem fixed before actually reviewing the patch.
(In reply to comment #14) > (In reply to comment #9) > > The problem is initial refresh of range is not called if it contains xforms > > label. I can't get the mystics but I think the things in patch go right. > > > I'd like know the reason for this and get the problem fixed before actually > reviewing the patch. > The reason is constructor of binding is not called.
If I change output to input and then hit the up arrow, then the constructor will be called. Really wierd. All I can suggest is posting to the xbl NG and asking there how to proceed. I know all of the bindings should attach and constructors run before onload, but I don't know if XUL gets onload or not. You could also try this patch on an older tree. Maybe try it on FF 1.5.x (since it is just chrome files you shouldn't have to build the browser if you repackage the chrome). Maybe this is a recent xbl regression. I checked the bugs and didn't see anything that looked right, though.
I debugged this some more using my testcase that has a xf:input instead of xf:output. Looks like there are two sets of bindings in this testcase. The first set of bindings get added to the attached queue for the binding manager and they will eventually get processed in nsBindingManager::ProcessAttachedQueue. These are the bindings for xul:textbox and xf:label (4 of them) and xf:input. The processing is called by nsCSSFrameConstructor::ContentInserted which was itself called by PresShell::InitialReflow. Then I see xul:textbox, xf:input and 3 xf:ranges getting added to a binding manager's queue and that queue is never processed. This happens during a call stack that is started by nsCSSFrameConstructor::RestyleEvent::Run. The whole call stack looks like this: > gklayout.dll!nsBindingManager::AddToAttachedQueue(nsXBLBinding * aBinding=0x044a2ce0) Line 765 C++ gklayout.dll!nsAutoEnqueueBinding::~nsAutoEnqueueBinding() Line 1881 C++ gklayout.dll!nsCSSFrameConstructor::ConstructFrameInternal(nsFrameConstructorState & aState={...}, nsIContent * aContent=0x045b3dc0, nsIFrame * aParentFrame=0x04fae840, nsIAtom * aTag=0x03b14358, int aNameSpaceID=0x00000009, nsStyleContext * aStyleContext=0x04fcb6f8, nsFrameItems & aFrameItems={...}, int aXBLBaseTag=0x00000000) Line 8056 + 0x19 bytes C++ gklayout.dll!nsCSSFrameConstructor::ConstructFrame(nsFrameConstructorState & aState={...}, nsIContent * aContent=0x045b3dc0, nsIFrame * aParentFrame=0x04fae840, nsFrameItems & aFrameItems={...}) Line 7989 + 0x35 bytes C++ gklayout.dll!nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState & aState={...}, nsIContent * aContent=0x03c10290, nsIFrame * aFrame=0x04fae840, int aCanHaveGeneratedContent=0x00000000, nsFrameItems & aFrameItems={...}, int aParentIsBlock=0x00000000, nsTableCreator * aTableCreator=0x00000000) Line 11818 + 0x3a bytes C++ gklayout.dll!nsCSSFrameConstructor::ConstructXULFrame(nsFrameConstructorState & aState={...}, nsIContent * aContent=0x03c10290, nsIFrame * aParentFrame=0x04faa264, nsIAtom * aTag=0x00b34410, int aNameSpaceID=0x00000010, nsStyleContext * aStyleContext=0x04fadf7c, nsFrameItems & aFrameItems={...}, int aXBLBaseTag=0x00000000, int aHasPseudoParent=0x00000000, int * aHaltProcessing=0x0012f5fc) Line 6566 + 0x1e bytes C++ gklayout.dll!nsCSSFrameConstructor::ConstructFrameInternal(nsFrameConstructorState & aState={...}, nsIContent * aContent=0x03c10290, nsIFrame * aParentFrame=0x04faa264, nsIAtom * aTag=0x00b34410, int aNameSpaceID=0x00000010, nsStyleContext * aStyleContext=0x04fadf7c, nsFrameItems & aFrameItems={...}, int aXBLBaseTag=0x00000000) Line 8121 + 0x38 bytes C++ gklayout.dll!nsCSSFrameConstructor::ConstructFrame(nsFrameConstructorState & aState={...}, nsIContent * aContent=0x03c10290, nsIFrame * aParentFrame=0x04faa264, nsFrameItems & aFrameItems={...}) Line 7989 + 0x35 bytes C++ gklayout.dll!nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState & aState={...}, nsIContent * aContent=0x045dd3c0, nsIFrame * aFrame=0x04faa264, int aCanHaveGeneratedContent=0x00000001, nsFrameItems & aFrameItems={...}, int aParentIsBlock=0x00000000, nsTableCreator * aTableCreator=0x00000000) Line 11818 + 0x3a bytes C++ gklayout.dll!nsCSSFrameConstructor::ConstructDocElementFrame(nsFrameConstructorState & aState={...}, nsIContent * aDocElement=0x045dd3c0, nsIFrame * aParentFrame=0x04fa9f00, nsIFrame * * aNewFrame=0x0012f888) Line 4673 C++ gklayout.dll!nsCSSFrameConstructor::ReconstructDocElementHierarchyInternal() Line 8256 + 0x24 bytes C++ gklayout.dll!nsCSSFrameConstructor::ReframeContainingBlock(nsIFrame * aFrame=0x04fae7c8) Line 13319 C++ gklayout.dll!nsCSSFrameConstructor::MaybeRecreateContainerForIBSplitterFrame(nsIFrame * aFrame=0x04fae7c8, unsigned int * aResult=0x0012f9d4) Line 11645 + 0xc bytes C++ gklayout.dll!nsCSSFrameConstructor::RecreateFramesForContent(nsIContent * aContent=0x0467aee0) Line 11678 + 0x10 bytes C++ gklayout.dll!nsCSSFrameConstructor::RestyleElement(nsIContent * aContent=0x0467aee0, nsIFrame * aPrimaryFrame=0x04fae7c8, nsChangeHint aMinHint=0x00000000) Line 10550 C++ gklayout.dll!nsCSSFrameConstructor::ProcessOneRestyle(nsIContent * aContent=0x0467aee0, nsReStyleHint aRestyleHint=eReStyle_Self, nsChangeHint aChangeHint=0x00000000) Line 13402 C++ gklayout.dll!nsCSSFrameConstructor::ProcessPendingRestyles() Line 13455 C++ gklayout.dll!nsCSSFrameConstructor::RestyleEvent::Run() Line 13518 C++ xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=0x00000001, int * result=0x0012fbc4) Line 483 C++ xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00abb220, int mayWait=0x00000001) Line 225 + 0x16 bytes C++ gkwidget.dll!nsBaseAppShell::Run() Line 153 + 0xc bytes C++ tkitcmps.dll!nsAppStartup::Run() Line 171 + 0x1c bytes C++ xul.dll!XRE_main(int argc=0x00000001, char * * argv=0x00ab7c58, const nsXREAppData * aAppData=0x004036b0) Line 2465 + 0x25 bytes C++ firefox.exe!main(int argc=0x00000001, char * * argv=0x00ab7c58) Line 61 + 0x13 bytes C++ firefox.exe!__tmainCRTStartup() Line 586 + 0x19 bytes C firefox.exe!mainCRTStartup() Line 403 C kernel32.dll!7c816fd7() [Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll] So I'm guessing that there is a whole in the xbl logic somewhere.
All works fine if I add binding just for range. But if I add binding by @typelist using then any child element of range brokes it.
Boris, probably you can help to figure out why binding constructor is not called (see aaron's comment #17).
Not until sometime next week, probably after Thursday next week... I'll put it on my list.
Though come to think of it, is the issue that nsCSSFrameConstructor::ReconstructDocElementHierarchyInternal doesn't process binding constructors and needs to do it?
(In reply to comment #21) > Though come to think of it, is the issue that > nsCSSFrameConstructor::ReconstructDocElementHierarchyInternal doesn't process > binding constructors and needs to do it? > That's kinda what I was thinking...that anyone who called nsCSSFrameConstructor::ConstructDocElementFrame should have to then process that document's binding manager's attached queue. But I don't know the code well enough to generalize like that.
Either the callers need to, or that function needs to. Someone would need to trace all possible codepaths to it to see whether just doing it in the function would work.
Attached patch Does this help? (obsolete) — Splinter Review
Try this. Does it help?
(In reply to comment #24) > Created an attachment (id=241516) [edit] > Does this help? > > Try this. Does it help? > I get compile error: d:/Mozilla\mozilla\layout\base\nsCSSFrameConstructor.cpp(8284) : error C2065: 'a utoLayoutPhase' : undeclared identifier d:/Mozilla\mozilla\layout\base\nsCSSFrameConstructor.cpp(8284) : error C2228: le ft of '.Exit' must have class/struct/union type type is ''unknown-type'' d:/Mozilla\mozilla\layout\base\nsCSSFrameConstructor.cpp(8286) : error C2228: le ft of '.Enter' must have class/struct/union type type is ''unknown-type'' d:/Mozilla\mozilla\layout\base\nsCSSFrameConstructor.cpp(8286) : error C3861: 'a utoLayoutPhase': identifier not found, even with argument-dependent lookup make[5]: *** [nsCSSFrameConstructor.obj] Error 2 Line 8284 contains LAYOUT_PHASE_TEMP_EXIT();
No longer blocks: 334603
Oh, hmm. Remove the layout phase lines or do a non-debug build? Getting the layout phase stuff working with this will be .... interesting.
(In reply to comment #26) > Oh, hmm. Remove the layout phase lines or do a non-debug build? Getting the > layout phase stuff working with this will be .... interesting. > My patch works fine if I apply your patch without layout phase lines. Thank you a lot.
Attached patch patch4 (obsolete) — Splinter Review
Olli, you can return to patch reviewing. You can test it after bz's patch applying.
Attachment #240180 - Attachment is obsolete: true
Attachment #241547 - Flags: review?(Olli.Pettay)
Attachment #240180 - Flags: review?(Olli.Pettay)
So we should figure out how to get the layout phase stuff working properly here. Alexander, I don't really have time to do that. Do you?
(In reply to comment #29) > So we should figure out how to get the layout phase stuff working properly > here. Alexander, I don't really have time to do that. Do you? > I guess I have time but I don't know code you are patching. Why is layout phase stuff needed?
To warn/assert if non-reentrant code (most of frame constructor) is reentered.
Add a parameter for the auto layout phase object to ReconstructDocElementHierarchyInternal, defaulting to null. Then, ifdef-DEBUG, null-check it and do the TEMP_ENTER and TEMP_EXIT around the calls.
Of course, that doesn't fix the other callers -- but I'm thinking it probably shouldn't. Maybe the ProcessAttachedQueue call should be in ReconstructDocElementHierarchy instead, since the other callers of ReconstructDocElementHierarchyInternal will call it after unwinding?
Actually, the other callers of ReconstructDocElementHierarchyInternal will not call it after unwinding. For example, WipeContainingBlock calls ReconstructDocElementHierarchyInternal and doesn't process the attached queue. So will ReframeContainingBlock. ReframeContainingBlock is called from ContentAppended, and ContentAppended doesn't process after calling it. That sort of thing. I did trace all the callstacks, and in no case when we get into ReconstructDocElementHierarchyInternal do we process the queue, as far as I can tell. Adding an arg would work; we'd need to add similar args to ReframeContainingBlock and WipeContainingBlock.
Aren't WipeContainingBlock and ReframeContainingBlock called from the normal frame construction entry points, which will themselves call ProcessAttachedQueue? I think the idea is that ProcessAttachedQueue should be called by the entry point into frame construction -- that's how we manage not to crash when it does bad things.
That is, called *indirectly* from the normal frame construction entry points, via the normal recursion.
> Aren't WipeContainingBlock and ReframeContainingBlock called from the normal > frame construction entry points Yes, but the code looks like this (several examples coming): nsCSSFrameConstructor::ContentAppended (which seems to have a bug in the case when the parent is a frameset, btw -- doesn't temp-exit things): if (needReframe) return ReframeContainingBlock(parentFrame); .... // Process the attached queue .... if (WipeContainingBlock(state, containingBlock, parentFrame, frameItems.childList)) { return NS_OK; } nsCSSFrameConstructor::ContentInserted (looks like it needs to exit/reenter around the ReinsertContent call if parent is special): if (NeedSpecialFrameReframe(aContainer, container, parentFrame, aChild, aIndexInContainer, prevSibling, nextSibling)) { return ReframeContainingBlock(parentFrame); } .... // Process the attached queue .... if (WipeContainingBlock(state, containingBlock, parentFrame, frameItems.childList)) return NS_OK; That sort of thing.
Er, ok, I was right the first time, when I forgot to mention the recursion I was thinking about... So I think the callers need to anyway, for the case where we didn't have to reconstruct the root element, so I still think that's the right answer -- we just need to add the calls.
OK. I think that would be fine, though I should note that the current setup where we end up calling ReinsertContent will effectively reenter frame construction, no? We should probably not count it as an entry point if ContentInserted is called from ReinsertContent.
Attached patch layout patch (obsolete) — Splinter Review
I tried to call ProcessAttachedQueue() only for ReconstructDocElementHierarchy() and for WipeContainingBlock() but it didn't help. In this patch ProcessAttachedQueue() is called for ReframeContainingBlock() too and that works.
Attachment #241516 - Attachment is obsolete: true
Attachment #246302 - Flags: review?(bzbarsky)
Comment on attachment 246302 [details] [diff] [review] layout patch The layout phase stuff here is wrong. We really do need to propagate in the layout phase tracker from the callers if we want to do it right.
Attachment #246302 - Flags: review?(bzbarsky) → review-
Attached patch layout patch2 (obsolete) — Splinter Review
like this?
Attachment #246302 - Attachment is obsolete: true
Attachment #246309 - Flags: review?(bzbarsky)
Comment on attachment 246309 [details] [diff] [review] layout patch2 Yes, except you shouldn't be adding any entry points. All existing entry points need to propagate in their autoLayoutPhase.
Attachment #246309 - Flags: review?(bzbarsky) → review-
(In reply to comment #43) > (From update of attachment 246309 [details] [diff] [review] [edit]) > Yes, except you shouldn't be adding any entry points. All existing entry > points need to propagate in their autoLayoutPhase. > ReconstructDocElementHierarchy() - autoLayoutPhase is generated right there WipeContainingBlock() - autoLayoutPhase is generated in ContentAppended()/ContentInserted That's fine. But stack for ReframeContainingBlock() calls is looked like: ReframeContainingBlock ContentAppended - autoLayoutPhase yes ContentInserted - autoLayoutPhase yes ContentRemoved - autoLayoutPhase yes MaybeRecreateContainerForIBSplitterFrame RecreateFramesForContent ContentAppended - autoLayoutPhase yes ContentInserted - autoLayoutPhase yes ContentRemoved - autoLayoutPhase yes ProcessRestyledFrames RestyleElement ProcessOneRestyle AttributeChanged ProcessPendingRestyles RestyleEvent::Run RestyleElement RestyleLaterSiblings ProcessOneRestyle ProcessOneRestyle MaybeRecreateFramesForContent RestyleElement It looks like some points of this hierarchy doesn't have autoLayoutPhase object. Then should I create it and where it's better to do it?
RecreateFramesForContent might need to create one. Check with dbaron?
(In reply to comment #45) > RecreateFramesForContent might need to create one. Check with dbaron? > Should I pass existed autoLayoutPhase object into RecreateFramesForContent() if RecreateFramesForContent() was called from ContentAppended/ContentInserted/ContentRemoved?
Hmm... Not sure. Perhaps those should exit their layout phases before calling RecreateFramesForContent.
(In reply to comment #47) > Hmm... Not sure. Perhaps those should exit their layout phases before calling > RecreateFramesForContent. > Do you mean I should call RecreateFramesForContent after any LAYOUT_PHASE_TEMP_REENTER call but before LAYOUT_PHASE_TEMP_EXIT? Then what is difference between using of RecreateFramesForContent() and WipeContainingBlock()/ReframeContainingBlock()? The code looks like WipeContainingBlock()/ReframeContainingBlock() never are called between LAYOUT_PHASE_TEMP_EXIT() and LAYOUT_PHASE_TEMP_REENTER() inside of contentAppended/e.t.c. Btw, contentAppended calls contentInserted inside layout phase macros and contentInserted creates autoLayoutPhase object that calls layout phase macros too (http://lxr.mozilla.org/mozilla/source/layout/base/nsCSSFrameConstructor.cpp#8696). Is this wrong?
> Do you mean I should call RecreateFramesForContent after any > LAYOUT_PHASE_TEMP_REENTER call but before LAYOUT_PHASE_TEMP_EXIT? No, the other way around. > Then what is difference between using of RecreateFramesForContent() and > WipeContainingBlock()/ReframeContainingBlock()? The former is called from outside frame construction code; the latter are not. > Btw, contentAppended calls contentInserted inside layout phase macros and It exits the phase before the call, no? And reenters it afterward?
Attached patch layout patch3 (obsolete) — Splinter Review
Attachment #246309 - Attachment is obsolete: true
Attachment #247817 - Flags: review?(bzbarsky)
I doubt I'll get to this before January.
Attached patch patch5 (updated)Splinter Review
Attachment #241547 - Attachment is obsolete: true
Attachment #241547 - Flags: review?(Olli.Pettay)
Attached patch layout patch4 (updated) (obsolete) — Splinter Review
Attachment #247817 - Attachment is obsolete: true
Attachment #251778 - Flags: review?(bzbarsky)
Attachment #247817 - Flags: review?(bzbarsky)
That patch won't even build in a non-debug build, so I question how much it's been tested.
Comment on attachment 251778 [details] [diff] [review] layout patch4 (updated) Other than that minor issue, this does look ok, but that issue needs to be addressed..
Attachment #251778 - Flags: review?(bzbarsky) → review-
Attached patch layout patch5Splinter Review
(In reply to comment #55) > (From update of attachment 251778 [details] [diff] [review]) > Other than that minor issue, this does look ok, but that issue needs to be > addressed.. > This patch should work for release, though I failed to test it because I got error that isn't related with the patch: Creating library nspr4.lib and object nspr4.exp make[5]: *** No rule to make target `nspr4.pdb', needed by `export'. Stop. make[5]: Leaving directory `/cygdrive/d/Mozilla/browser/obj-i586-pc-msvc/nsprpub /pr/src' Boris, may I ask you to see if it works for release again?
Attachment #251778 - Attachment is obsolete: true
Attachment #252336 - Flags: review?(bzbarsky)
Blocks: 367826
(In reply to comment #56) > Boris, may I ask you to see if it works for release again? > Boris, I tested on debug/release. Looks ok.
So out of curiousity... Does the patch in bug 267833 by any chance help here? I'm not sure when I'll have time to trace through this stuff enough to convince me that this patch is right, so.... :(
Comment on attachment 252336 [details] [diff] [review] layout patch5 r- pending answer to the question in comment 58.
Attachment #252336 - Flags: review?(bzbarsky) → review-
I'm not working on this anymore.
Assignee: surkov.alexander → nobody
RIP xforms
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: