574 bytes, text/html
341 bytes, text/html
8.01 KB, patch
karnaze (gone): review+
karnaze (gone): superreview+
|Details | Diff | Splinter Review|
2002041003/Win2K Repro: 1. open attached page 2. edit document -> in 'Normal' is visible table 3. switch to 'Show All Tags' -> table is still visible 4. switch back to 'Normal' Actual: Table disappear. Switch again to 'Show All Tags' and table isn't there too. But in '<HTML> Source' are table's tags still visible/editable. Expected: Table still appear. Table is still visible.
Assignee: syd → brade
Created attachment 78897 [details] reduced testcase based on attachment 78702 [details]
Attachment #78702 - Attachment is obsolete: true
The problem in the html testcase is the <col> tags. Without the col tags I don't see a problem. This appears to be a problem with our ShowAllTags stylesheet. -->glazman cc karnaze since Daniel's fix is just a workaround and the real problem is in TBODY / layout.
Assignee: brade → glazman
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: --- → mozilla1.0.1
Created attachment 78899 [details] [diff] [review] patch v1.0 Workaround for EditorAllTags.css. Please review. Chris : what in the rule I am commenting out for TBODY could cause the problem ?
Comment on attachment 78899 [details] [diff] [review] patch v1.0 r=brade if you remove the two spaces before tbody (so that line won't appear touched)
Attachment #78899 - Flags: review+
Comment on attachment 78899 [details] [diff] [review] patch v1.0 email@example.com Since brade brought up the space thing ... :-P ... you might as well move your closing */ to it's own line too so that all that shows in the diff is what you added.
Attachment #78899 - Flags: superreview+
Daniel, I'm not familar with the file being pached.
Taking the bug.
Assignee: glazman → karnaze
Severity: normal → major
Priority: -- → P1
Target Milestone: mozilla1.0.1 → mozilla1.0
Created attachment 78983 [details] [diff] [review] patch to not treat row groups as siblings of col groups or cols during frame construction This bug will affect pages outside of composer. The patch fixes regression tests table/dom/appendCol1.html, appendColGroup1.html.
Attachment #78899 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 78983 [details] [diff] [review] patch to not treat row groups as siblings of col groups or cols during frame construction sr=attinasi, but you need to ad an |XXX| indicating that this will not work when an element that is mapped to a |rowgroup| via CSS is inserted. Probably even need to create another bug on that, though it seems like it will be expensive to handle that case because you have to resolve style to figure it out...
Attachment #78983 - Flags: superreview+
Created attachment 79470 [details] [diff] [review] revised patch to use display type rather than html tag
Attachment #78987 - Attachment is obsolete: true
Comment on attachment 79470 [details] [diff] [review] revised patch to use display type rather than html tag In the method IsValidSibling you need to fix the scoping of the StyleContext so that it does not get destroyed before you check the display structure. You are just getting lucky now because the contexts are arena allocated I think. Fix that (by moving the styleContext COMPtr to the outer scoping block) and sr=attinasi
Comment on attachment 79470 [details] [diff] [review] revised patch to use display type rather than html tag withdrawing SR: Kin pointed out, correctly, that you cannot pass the styleDisplay back out of the method IsValidSibling since you are not holding a ref to the context that it belongs to (sorry, I missed that). You need to either give up on caching the display structure and jsut cache the display value, or else make sure you hold a ref to the context longer too.
Created attachment 79494 [details] [diff] [review] revised patch with reviewers' suggestions
Attachment #79470 - Attachment is obsolete: true
Comment on attachment 79494 [details] [diff] [review] revised patch with reviewers' suggestions Cool - I guess it will be a long time before 255 is a valid NS_STYLE_DISPLAY value... sr=attinasi
Attachment #79494 - Flags: superreview+
Comment on attachment 79494 [details] [diff] [review] revised patch with reviewers' suggestions firstname.lastname@example.org with some points to consider ... not necessarily requirements: - Since we only ever use aSiblingDisplay.mDisplay inside of IsValidSibling(), do we want to replace the aSiblingDisplay arg: + const nsStyleDisplay& aSiblingDisplay, with something like: + PRUint8 aSiblingDisplayValue, to be consistent with the aDisplayValue arg? - The pairing of the aSibling frame arg with an aContent node seems kind of odd, should IsValidSibling() ever be used outside of the FindNext/PreviousSibling(), we should consider moving the responsibility to calculate the display context to the caller, which would make the method more generic/reuseable. That would allow us to remove the presShell, frame and content args and remove the need for UNSET_DISPLAY.
Attachment #79494 - Flags: review+
Kin, I like your first suggestion. I didn't want the caller to have to construct the style context to get the display because it happens in two places. Also, the sibling frame is needed to get the parent frame. So, I think having that code in one place is better than duplicating it to save some parameter passing.
Keywords: nsbeta1 → nsbeta1+
Created attachment 79761 [details] [diff] [review] revised patch with additional reviewer's suggestions
Pls land this on the trunk, and have QA verify the fix, and make sure it causes no regressions. thanks!
Whiteboard: PATCH → PATCH [adt2]
The patch went into the trunk 4/18/2 pm.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Whiteboard: PATCH [adt2] → PATCH [adt2][FIXED_ON_TRUNK]
verified on 4/19 trunk build. looks good I see the table when toggling back and forth between Normal and Show all tags...
Status: RESOLVED → VERIFIED
The risk here to css seems greater than the # of people who will benifit from this fix for MachV beta. Let's leave it on the trunk and take it for RTM. = adt1.0.0-/[adt2 RTM]
Keywords: adt1.0.0 → adt1.0.0-
Whiteboard: PATCH [adt2][FIXED_ON_TRUNK] → PATCH [adt2 RTM][FIXED_ON_TRUNK]
*** Bug 143268 has been marked as a duplicate of this bug. ***
renominating for RTM
Keywords: adt1.0.0- → adt1.0.0
adding adt1.0.0-. Let's get this into the next release.
Keywords: adt1.0.0 → adt1.0.0-
Adding custrtm-; no impact on customization.
Whiteboard: PATCH [adt2 RTM][FIXED_ON_TRUNK] → PATCH [adt2 RTM][FIXED_ON_TRUNK],custrtm-
You need to log in before you can comment on or make changes to this bug.