table disappear after switch between normal edit and show all tags

VERIFIED FIXED in mozilla1.0

Status

SeaMonkey
Composer
P1
major
VERIFIED FIXED
16 years ago
14 years ago

People

(Reporter: Adam Hauner, Assigned: karnaze (gone))

Tracking

({dataloss})

Trunk
mozilla1.0
dataloss

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PATCH [adt2 RTM][FIXED_ON_TRUNK],custrtm-)

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

16 years ago
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.
(Reporter)

Comment 1

16 years ago
Created attachment 78702 [details]
Testcase page

Comment 2

16 years ago
--> brade/cmanske
Assignee: syd → brade

Comment 3

16 years ago
Created attachment 78897 [details]
reduced testcase based on attachment 78702 [details]
Attachment #78702 - Attachment is obsolete: true

Comment 4

16 years ago
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 6

16 years ago
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 7

16 years ago
Comment on attachment 78899 [details] [diff] [review]
patch v1.0

sr=kin@netscape.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+
(Assignee)

Comment 8

16 years ago
Daniel, I'm not familar with the file being pached.
(Assignee)

Comment 9

16 years ago
Created attachment 78922 [details]
minimal test case
(Assignee)

Comment 10

16 years ago
Taking the bug.
Assignee: glazman → karnaze
Severity: normal → major
Keywords: dataloss
Priority: -- → P1
Target Milestone: mozilla1.0.1 → mozilla1.0
(Assignee)

Comment 11

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

Updated

16 years ago
Status: NEW → ASSIGNED
Keywords: nsbeta1
Whiteboard: PATCH

Comment 12

16 years ago
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+
(Assignee)

Comment 13

16 years ago
Created attachment 78987 [details] [diff] [review]
revised patch
(Assignee)

Updated

16 years ago
Attachment #78983 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #78987 - Flags: superreview+
(Assignee)

Comment 14

16 years ago
Created attachment 79470 [details] [diff] [review]
revised patch to use display type rather than html tag
Attachment #78987 - Attachment is obsolete: true

Comment 15

16 years ago
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
Attachment #79470 - Flags: superreview+

Comment 16

16 years ago
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.
Attachment #79470 - Flags: superreview+
(Assignee)

Comment 17

16 years ago
Created attachment 79494 [details] [diff] [review]
revised patch with reviewers' suggestions
Attachment #79470 - Attachment is obsolete: true

Comment 18

16 years ago
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 19

16 years ago
Comment on attachment 79494 [details] [diff] [review]
revised patch with reviewers' suggestions

r=kin@netscape.com 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+
(Assignee)

Comment 20

16 years ago
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. 
nsbeta1+
Keywords: nsbeta1 → nsbeta1+
(Assignee)

Comment 22

16 years ago
Created attachment 79761 [details] [diff] [review]
revised patch with additional reviewer's suggestions
(Assignee)

Updated

16 years ago
Attachment #79761 - Flags: superreview+
Attachment #79761 - Flags: review+
(Assignee)

Updated

16 years ago
Attachment #79494 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Keywords: adt1.0.0, approval

Comment 23

16 years ago
Pls land this on the trunk, and have QA verify the fix, and make sure it causes
no regressions. thanks!
Whiteboard: PATCH → PATCH [adt2]
(Assignee)

Comment 24

16 years ago
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]

Comment 25

16 years ago
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

Comment 26

16 years ago
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]

Comment 27

16 years ago
*** Bug 143268 has been marked as a duplicate of this bug. ***

Comment 28

16 years ago
renominating for RTM
Keywords: adt1.0.0- → adt1.0.0

Comment 29

16 years ago
adding adt1.0.0-.  Let's get this into the next release.
Keywords: adt1.0.0 → adt1.0.0-

Comment 30

16 years ago
Adding custrtm-; no impact on customization.

Updated

16 years ago
Whiteboard: PATCH [adt2 RTM][FIXED_ON_TRUNK] → PATCH [adt2 RTM][FIXED_ON_TRUNK],custrtm-
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.