Closed Bug 39098 Opened 24 years ago Closed 14 years ago

Elements with visibility:hidden, visibility:collapse, or display:none get copied to the clipboard

Categories

(Core :: DOM: Serializers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5
Tracking Status
blocking2.0 --- -

People

(Reporter: eXv, Assigned: MatsPalmgren_bugz)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Whiteboard: [Hixie-P0][firebug-p2])

Attachments

(9 files, 8 obsolete files)

719 bytes, text/html
Details
747 bytes, text/html
Details
1.98 KB, text/html
Details
2.92 KB, text/html
Details
517 bytes, text/html
Details
21.40 KB, patch
Details | Diff | Splinter Review
2.78 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
4.16 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
11.04 KB, patch
Details | Diff | Splinter Review
DESCRIPTION:
Elements with the CSS property visibility set to hidden or collapse show up if
you copy text to the clipboard.

STEPS TO REPRODUCE:

If you have code like this:

<html><head><title>invisible copies</title>
<style type="text/css">
.hideme{
   visibility: hidden;
	
}

.trclass {
  visibility: collapse;
	
}
</style>
</head>

<body>
<div class="hideme">Hide me, please hide me</div>
<table>
 <tr class="trclass">
   <td>I would like to be hidden as well, please.</td>
 </tr>
</table>
Just me on this page...Nobody else.
</body>
</html>

Now, Select All text, Copy, Paste,
all the text shows up.

The provided URL gives a better demonstration

http://131.96.115.58/~exv/moztests/invisible-elements-get-copied-to-clipboard.html
Sorry, running Linux Build ID 2000051120
	
Seeing this with NT and 2000051108.
not sure if this is Style System, Selection or clipboard problem  Over to Style 
System first.
Assignee: asadotzler → pierre
Status: UNCONFIRMED → NEW
Component: Browser-General → Style System
Ever confirmed: true
QA Contact: jelwell → chrisd
Hmmm - I wonder if that is even a bug. I noticed IE5 on Windows does the same 
thing. 

The CSS2 spec says that visibility:hidden and visibility:collapse make the 
element's box invisible, however the box still affects layout. Hence, the 
elements are really there they just cannot be seen. Using visibility: none on 
the other hand will make it so the element does not even exist and in that case 
it will nto be selected.

I think it is up to Selection to determine their preferred semantic: should we 
select hidden and collapsed elements like IE does, or should we skip them?

Over to selection...
Assignee: pierre → mjudge
Component: Style System → Selection
QA Contact: chrisd → elig
Is it a bug: It depends if copy&paste is rendering the document to text, or if
it is providing the user with a pretty-printed copy of the DOM.

I would have thought it was the first, and thus visibility:hidden items would 
not be copied. Items with display:none certainly should not be copied, I'm sure 
we all agree.

I think this should go over to the UI folk for a sanity check.
Pierre -- could use your input on this one
Assignee: mjudge → pierre
I agree with Marc and Ian. It's a question of preference as well as of common 
sense. I think that invisible elements should not be copied to the clipboard when 
'mUserSelect' is set to NS_STYLE_USER_SELECT_TEXT. These elements should ignore 
the selection as if 'mUserSelect' were set to NS_STYLE_USER_SELECT_NONE. 
Reassigned to Selection.

Note: the problem is different in the Editor, where one may expect all the 
elements to be copied/pasted.
Assignee: pierre → mjudge
thanks Pierre -- setting to m17 for now
Target Milestone: --- → M17
per beppe:
Status: NEW → ASSIGNED
Target Milestone: M17 → Future
adding help wanted to the keywords
Keywords: helpwanted
OS: Linux → All
Hardware: PC → All
I've stumbled across this in MacOS 9 too, and the test case works as advertized
in MacOS 8.6 (2000081109).  All in all, this is cross-platform.
*SPAM*: Changing the QA contact of all open/resolved Selection bugs from 
elig@netscape.com to BlakeR1234@aol.com.  After the many great years of service 
Eli has given to Mozilla, it's time for him to move on; he has accepted a 
position at Eazel.  We'll be sad to see him go, and I'll do my best to fill his 
spot...
QA Contact: elig → BlakeR1234
Blocks: 57770
*** Bug 57808 has been marked as a duplicate of this bug. ***
*** Bug 58625 has been marked as a duplicate of this bug. ***
Summary: CSS Elements with visibility hidden or collapse get copied to the clipboard → CSS Elements with visibility hidden or collapse and display:none get copied to the clipboard
not going to check style when we copy. leaving here in future
I'd like to confirm bug 39098 - Mozilla  and Netscape 6.01on a Mac OS X 
(10.0.4) system both show message that should be hidden via css class 
item 'display: none'. The Mozilla build ID is 2001061314. I also rebooted 
into OS 9.1, and reproduced this problem using the Netscape 6.01 
(Gecko/20010131) . This message appears whenever relevant page gets 
loaded in either of these browsers.


css code:
  .ahem {
       display: none;
   }

xhtml code:
  <h1 class="ahem">This site will look much better in a browser
      that supports <a href="http://www.webstandards.org/upgrade/"
      title="Download a browser that complies with Web standards.">web
      standards</a>, but it is accessible to any browser or Internet
      device.</h1>
changing selection qa to tpreston.
QA Contact: blaker → tpreston
*** Bug 52189 has been marked as a duplicate of this bug. ***
removing myself from the cc list
if you actually copied the instructions from the url into your shell, you'd be
very unhappy. IMO we should fix this before 1.2 is released.
Blocks: errorpages
This bug makes it futile to copy and page errors from the spiffy new error
pages, do to large amounts of hidden text. (not to mention the security issue
the URL demonstrates)
This bug is a must-fix for any web author implementing folding sections or list
items through JS and visibility/display CSS properties. And has the example URL
shows it, there could be security issues if a site says to the visitor "copy
this line and paste it into your shell window". Ok, a light security issue but
still.
This bug isn't a security hole.  A web page could still mislead a user as to
what is being copied without this bug.
Whiteboard: [Hixie-P0]
*** Bug 99159 has been marked as a duplicate of this bug. ***
This really isn't a selection bug; it's a clipboard issue.  I'm retargetting to
component: DOM to Text Conversion but that probably isn't the best component either.
Assignee: mjudge → harishd
Status: ASSIGNED → NEW
Component: Selection → DOM to Text Conversion
QA Contact: tpreston → sujay
*** Bug 222344 has been marked as a duplicate of this bug. ***
Can we get a better assignee?  Mkaply, anyone?

/be
When this issue has come up in other bugs, it hinged on there being no fast way
to get from content to frames to test visibility (so the serializers hack around
that, and try to guess).  A slow frame check for each content item can mean
increased page load times, and freezes when selecting text (if autocopy is on).
 But that was quite a while ago, and the performance problem may be fixed now. 
When testing patches for this bug, be sure to do lots of performance testing,
including selection with autocopy.
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.3) Gecko/20040910

This is also a problem when saving a page as text.  
1.  View <http://www.rossde.com/>.  
2.  Right-click and select "Save Page As".  
3.  On Save Page window, select "Text Files" as Save as type.  
4.  Save the page.  
5.  View the saved file with an ASCII viewer.  

Near the top of the text file, you will see "Note: My Web pages are best viewed
with style sheets enabled."  This is in a paragraph that has the style {display:
none}.  That paragraph was not seen in the Mozilla browser window (which does
not yet have an option to suppress CSS per bug #68416).  

While I suggest that this be controlled by an option, the default should be not
to display hidden text.  That is, the default should reflect the converse of
WYSIWYG in that "what you don't see is what you don't get".  
*** Bug 264998 has been marked as a duplicate of this bug. ***
Assignee: harishd → dom-to-text
Keywords: mozilla1.2
QA Contact: sujay
*** Bug 285629 has been marked as a duplicate of this bug. ***
The issue of display:none doesn't only affect copy/paste. It also affects «Find
in this page». The copy/paste issue causes the user to get more info than he
wants. The «Find in this page» issue cause the user to not locate what appears
to be on the page.  

The attached html file demonstrates the effect.
(In reply to comment #5)
> Items with display:none certainly should not be copied, I'm sure we all agree.
(In reply to comment #22)
> a must-fix for any web author [... And ...] could be security issues 

Quoting these seniors about this 5 year old bug, this junior adds: this is one
fuzzy bug report. It mixes the issue of display: none, where CSS2.1 author/Opera
developer Hixie tells us there is no doubt that Firefox _gravely_ breaks the CSS
specs, with the issues of collapsing and hiding elements, for which the case is
open for judgement and evaluation. 

When a report contains a mix of major and minor issues, the whole report should
be awarded awarded with the highest seriousity. Or else there should be opened a
separate bug for the issue of display: none.

Can we have someone with the right priveleges raising the severity of this bug?
It should be «Major» at the minimum. And it should have high priority for a
standards bases browser. This bug needs attention.
Summary: CSS Elements with visibility hidden or collapse and display:none get copied to the clipboard → Elements with visibility:hidden, visibility:collapse, or display:none get copied to the clipboard
*** Bug 302454 has been marked as a duplicate of this bug. ***
*** Bug 306022 has been marked as a duplicate of this bug. ***
This bug can be really irritating when making tab interfaces - when selecting the content of one tab, all the tabs' content gets copied to the clipboard.
This bug makes it very difficult to offer a database-oriented web application that allows row filtering and column hiding and also permits useful cutting and pasting into spreadsheets.  I came up with a workaround, involving a button that uses a script to individually select visible cells, but that has its own limitations.

Interestingly, Konqueror seems to have a similar but not identical problem (hidden rows don't get copied, but hidden columns do).  So there seems to be a general problem with this.
Your app could remove the "hidden" cells from the DOM entirely, no?
Yes, although that will certainly be a lot slower than simply hiding columns and such (we're talking in some cases tables with ~30 columns and thousands of rows).
those rows shouldn't be in the dom, they're expensive. do gecko (and the other browsers) a favor and leave them out until they belong :).

sadly, it doesn't seem that i've mentioned the other half of this bug. If I'm copying to an html editor, then i do want the hidden nodes, because I want to manipulate them.

My proposal is that we only drop invisible things when serializing to text. Jesse and I both have this approach for things we care about: copy from html, paste to "notepad", copy from notepad, paste to "rich target".

That solves one process question while not at all solving the speed questions akk raised in comment 28 (roc?).

http://mxr.mozilla.org/seamonkey/source/content/base/src/nsPlainTextSerializer.cpp
I'm hoping that this can be addressed by changing nsPlainTextSerializer::AppendText which has an nsIDOMText node.

I think in theory one can use that (chase some its parent nodes for an nsIDOMElement) and getComputedStyle to find out if the node is visible.
QA Contact: dom-to-text
I really doubt there are significant performance issues. AFAIK we don't do "autocopy" any more.
we still support autocopy, this i know. and some codepaths come from things like form controls being queried for values (although in those cases, they won't do anything interesting).

I know this bug itself doesn't care about generated content, but i'd like to point out that it exists and part of the problems we have are because not enough context is provided when a copy is made:
data:text/html,<head><title>test</title><style>#a+#b:after{content:" me"} </style></head><body><p id=a>don't select </p><p id=b>copy</p></body>

The goal for that testcase would be to copy "copy me". in most cases, if you manage to copy "don't select" and "copy", you have: <p id=a>don't select </p><p id=b>copy</p>

which isn't helpful, because the relevant css (the winning css) isn't in the selected portion. Assuming we did manage to keep the style rules around and you copied the entire "<p id=b>copy</p>" section, it wouldn't work anyway, because the sibling (#a) is necessary for the style rule to work.

The actual case I have doesn't rely on generated content (well, not css generated content). It relies on late css rules which hide content (which is of course what this bug is interested in).

http://mxr-test.landfill.bugzilla.org/mozilla-central/search?string=SharedObject&find=content for people who are curious.

Basically, from reading some stack traces and code, it seems in some cases we serialize some dom to an html string and then use an nsIParser with the NS_PLAINTEXTSINK_CONTRACTID to extract the "meaning". This can't work well in any of these cases. The gory code is nsCopySupport::HTMLCopy, I think it's all wrong, maybe glazou can help me write code that's less wrong :).
So here's what's going on: the particular application allows users to sort and filter the data, and hide columns.  Sort operations reorder the rows, while filter and hide operations simply make the rows and columns respectively invisible.

All of this stuff is dynamic.  It's all done in client-side JavaScript, which bogs down on really big reports but is still faster than reloading it from the server -- and more importantly, doesn't require any server-side support, which simplifies things considerably.

Dealing with removing (as opposed to hiding) both rows and columns (which will probably mean cells, since columns aren't really first-class objects) while also handling sorting, will be a lot of work.
yeah, i understand what you're trying to do, but ideally you would switch to removing nodes (leaving them in variables) and re-adding them in response to user pokes (no server hits required). Preferably start with the user interface which doesn't have content, get all the data you need as JSON (possibly in the original web page, it doesn't matter to me) or something and then insert only the bits you need. There are some DOM APIs to enable multiple edits to not trigger multiple reflows, which will help. But discussing your bits is beyond the scope of this bug. 
Is this bug for the fact that "CSS" formatting/styling doesn't get copied when you copy a page, or is this bug mostly about 'invisible elements'?

Bug 470785 was about css formatting not being copied -- and as a result, some invisible text was copied that told me why the formatting (that was copied) was messed up.
(or should I refile bug 470785 as a new bug that is more explicit about how Firefox throws away page formatting when copying.

That certainly seems like a fundamental browser flaw -- since IE has had the ability to copy & past rich text virtually from day 1.

You can paste copied text from IE into HTML-CSS,  rich-text editors, and formatting is usually preserved.

Seems like that's not the case with FF?

Unless I say "copy as text", I would expect to Copy/Paste to be WYSIWYG...
Bug 470785 is indeed not a duplicate of this bug, so I'll just reopen that bug for now. The more explicit you are in your description of the problem, the better. Especially, testcases that show the bug, would help.
Assignee: dom-to-text → nobody
Priority: P3 → --
We are experiencing the same issue in Firebug (hidden elements are selected when using Ctrl+A and consequently copied to the clipboard).

See more details here:
http://code.google.com/p/fbug/issues/detail?id=370

What is the chance to have this fixed?

Honza
Whiteboard: [Hixie-P0] → [Hixie-P0][firebug-p2]
Why is this a bug?  Technically, it's probably doing the right thing.

The problem is one of DESIGN -- what is the clipboard supposed to copy, plain text, or rich text?  Just because elements are hidden doesn't mean they shouldn't be copied -- they could be elements controlled by the style, and if not copied, would result in a bad copy to the destination.

The whole concept of the clipboard cut/paste needs to be more clearly defined and, enhanced, but I don't think the current behavior is a bug, in the sense that I believe it is doing what it is supposed to do.

I have the opposite complaint  about the clipboard's behavior.  When I cut and paste in IE, into Outlook, the styling of the text that was on a webpage, is correctly copied into the email message.  But with FF+TB, only the styling that's is present _in the elements_ you copy is collected.    

But that's not the style as it is displayed on the screen.

Sufficient information to reconstruct the full content (including hidden content) needs to be copied when the user wants to copy "rich text".  

When the user wants to copy "plain text" -- then non-visual elements need to be filtered out.
A plain text copy would still copy "visible objects" like pictures", but it would not copy the styling info.




As it stands, I don't think this is a 'BUG', but is an 'RFE' -- we all would like Copy to function one way or the other -- but that's not implemented 'yet'...  

I can see the benefit of having two types of copy -- but right now, the 'standard' copy is trying to do a rich-text copy by picking up all the elements (but it fails because it doesn't pickup the style that's applied to what I see).

There is an extension, I believe, https://addons.mozilla.org/en-US/firefox/addon/134, Plain Text Copy -- maybe that would do what you want and not copy the hidden content?
When you can't fix it, call it a feature! :-)
Re comment #58:  

There have been 476,643 downloads of the Plain Text Copy extension, which indicates a high interest in this capability.  

Further, the extension has not been updated for Firefox 3.5 or later, causing installation to be blocked for Firefox 3.5.  The same blocking might also prevent installation for non-Firefox Gecko browsers.  Thus, the extension is of doubtful validity for resolving this bug report.
Attached file Testcase #2
Attached patch Like so? (obsolete) — Splinter Review
Exclude content nodes that doesn't have a frame or CSS visibility != visible.
The detection of a collapsed table frame is a bit of a hack but I couldn't
find anything better.  I'm changing nsContentAreaDragDrop so that it uses
nsCopySupport (as the keyboard Copy command), I don't see why they should
use different code.
I assume I was cced to look at the patch in comment 62?

General questions:

1)  What do we mean by "collapsed" table frames here?  visibility:collapsed?
2)  Are we limiting to display+visibility because it's expedient, or because we
    think we do still want to be copying things with "position: absolute; left:
    -3000px" and such?
3)  Does this patch give the right behavior on:

      Copy 
      <span style="visibility: hidden">
        <span style="visibility: visible">all this<span>
      </span>
      text

    It seems like it might, given the code in SerializeToStringRecursive, but
    just checking.
Attached file Testcase #3
> I assume I was cced to look at the patch in comment 62?

Yes please, I meant to CC you first but messed it up...

> 1)  What do we mean by "collapsed" table frames here?

Yes, visibility:collapsed.

> 2)  Are we limiting to display+visibility because it's expedient...

I think display:none is by far the most common case.  It's not clear
to me what to do with clipped content; what if only some of the text is
clipped? or fully clipped children in a partly clipped ancestor?
scroll views? elements that are completely/partly obscured (overlapped)
by other elements?  I suspect that any fix for (some of) those would be
hard to understand so it seems better to rely on the author to specify
-moz-user-select:none or something to exclude it from the selection.

Bug 166235 is about copying -moz-user-select:none.  I could exclude
those in the patch here too but I don't think it's the right solution for
that case;  -moz-user-select:none shouldn't be included in the selection
range at all if I understand it correctly. (I could exclude them here as
a temporary wallpaper for that bug though).

Recent versions of IE, Opera, Safari and Chromium copies the clipped
content in your example.  There's a slight difference in how selection
of a display:none element is implemented:
Opera/Safari/Chromium includes it in the selection but removes it when
copying (both as text and HTML). 
Opera includes it in Selection.toString().
Safari/Chromium DOES NOT include it in Selection.toString().
FWIW, IE includes it in the selection and when copying.

I think Safari/Chromium does the right thing.  The attached patch
doesn't fix Selection.toString() though.

> 3)  Does this patch give the right behavior on: ...

Yes.
Attached patch Fixes Selection.toString() (obsolete) — Splinter Review
Together with one of the earlier patches, this makes us compatible with
Opera/Safari/Chromium as far as I can tell.
...this makes us compatible with Opera/Safari/Chromium when copying
*as plain text* I should say, we still include the display:none elements
when copying as HTML.
OK.  Skipping display:none and visibility:hidden/collapse seems reasonable.  Given that, some questions:

1)  Why not just refuse to descend into visibility:collapse table stuff, instead
    of trying to detect it on cells only and using sizing?  Are we trying to
    deal with cases where the selection is set programmatically to include part
    of a collapsed row or something?  Or is this to deal with collapsed
    columns?  Does height go to 0 when a column is collapsed?
2)  Why are we getting primary frames for _parents_ of textnodes instead of
    textnodes themselves?

I haven't read the details of the changes all around this code; just focusing on IsVisibleNode() for now.
(In reply to comment #70)
> 1)  Why not just refuse to descend into visibility:collapse table stuff,
> instead
>     of trying to detect it on cells only and using sizing?  Are we trying to

visibility:collapse applies to columns and column groups in addition to rows and row groups.  (That said, using sizing seems wrong.)
Mats, were you submitting those patches for review? At first glance, it looks like a good shot at a solution for this bug (and would help us for some hidden elements in Firebug as Honza suggested in comment #57).
Whiteboard: [Hixie-P0][firebug-p2] → [Hixie-P0][firebug-p2][needs-review]
Attached file Testcase #4
Attached patch Patch rev. 4 (obsolete) — Splinter Review
Thanks for your feedback, I think I've addressed most of your comments
with this patch.  There are still a few edge case issues, illustrated
in Testcase #4.  Bug 1: visibility:collapse from a <col> or <colgroup>
isn't detected in some cases. Bug 2 & 3: an explicit visibility:visible
inside a visibility:collapse row/rowgroup is taken as visible although
the content really isn't visible.  These first three bugs should be
very hard (impossible?) to encounter by mouse/keyboard selection, but
they can be triggered by setting up the selection by script.
Bug 4: the drag feedback is wrong (the copied text is correct though) -
I've tracked this down to "info->mBuilder.SetPaintAllFrames();"
in PresShell::CreateRangePaintInfo():
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5519
If I remove that line I get the correct drag feedback, but I'm 
assuming it's there for a reason.
Bug 5: unrelated bug

I think we can handle the above issues in separate bugs, they are edge
cases so shouldn't block this bug IMO.

Where do you prefer I set the "SkipInvisibleContent" flag,
in nsHTMLCopyEncoder or nsCopySupport (as in the 2nd patch)?
Assignee: nobody → matspal
Attachment #408280 - Attachment is obsolete: true
Attachment #408281 - Attachment is obsolete: true
Attachment #408418 - Attachment is obsolete: true
Attachment #408707 - Flags: review?(bzbarsky)
Attachment #408707 - Flags: review?(bzbarsky)
Comment on attachment 408707 [details] [diff] [review]
Patch rev. 4

Sorry, TryServer reports a regression with this patch in
content/events/test/test_dragstart.html:
initial selection text/plain - got
"This is a draggable  bit of text.", expected
"This is a draggable bit of text."

Investigating...
Attached file Testcase #5
All elements in that test is visible so it's a regression
from making the DnD code use the same code as kbd Copy.
There's an existing bug in that code that makes the
DnD mochitest fail.
Filed bug 524975 for the kbd Copy regression above.
Depends on: 524975
I have confirmed that the Plain Text Copy cited in comment #58 cannot be installed in SeaMonkey 2.0.
Comment on attachment 408707 [details] [diff] [review]
Patch rev. 4

This patch is correct but depends on bug 524975 to not break the test_dragstart.html mochitest.
Attachment #408707 - Flags: review?(bzbarsky)
Can we get an evaluation of whether this can land? We need to decide on the Firebug side if we have to find a workaround or just wait.
blocking2.0: --- → ?
Not going to block 1.9.3 on this, but yes, we should of course get this patch in, with an updated fix for bug 524975 as well.
blocking2.0: ? → -
(In reply to comment #81)
> Not going to block 1.9.3 on this, but yes, we should of course get this patch
> in, with an updated fix for bug 524975 as well.

Does this mean that status1.9.3 should be set to wanted?
Attachment #408701 - Attachment is patch: false
Attachment #408701 - Attachment mime type: text/plain → text/html
Comment on attachment 408707 [details] [diff] [review]
Patch rev. 4

I'll take this review request off of Boris's plate.
Attachment #408707 - Flags: review?(bzbarsky) → review?(dbaron)
Comment on attachment 408707 [details] [diff] [review]
Patch rev. 4

nsContentAreaDragDrop.cpp:

>-#include "nsIDocumentEncoder.h"
>-#include "nsIFrame.h"
>-#include "nsISelectionController.h"

It seems like there's still direct use of these classes in the file, so
you should probably keep them (though it doesn't matter that much).

>+extern nsresult NS_NewDomSelection(nsISelection **aDomSelection);

I don't think there's any need for |extern| here.

nsDocumentEncoder.cpp:

>+// Return true for a visibility:collapse row, row group, column,
>+// or column group (CSS 2.1, 11.2).
>+static PRBool
>+IsTableCollapsedFrame(nsIFrame* aFrame)
>+{
>+  if (aFrame->GetStyleVisibility()->mVisible != NS_STYLE_VISIBILITY_COLLAPSE)
>+    return PR_FALSE;
>+  const nsIAtom* const frameType = aFrame->GetType();
>+  return frameType == nsGkAtoms::tableRowFrame      ||
>+         frameType == nsGkAtoms::tableRowGroupFrame ||
>+         frameType == nsGkAtoms::tableColFrame      ||
>+         frameType == nsGkAtoms::tableColGroupFrame;
>+}

As I think has been pointed out before, this doesn't really handle
columns correctly, nor does it handle rowspanning cells whose first row
is collapsed but later rows are not.

Unfortunately the table code's handling of column collapsing is buried
within nsTableRowFrame::CollapseRowIfNecessary .  I'm actually a little
unsure whether we should even be attempting to handle table collapsing
when it's this inaccurate.  I'd probably slightly prefer taking it out
(and just having no special handling of collapsed on any table
elements), but I wouldn't argue strongly.

Either way, you should add comments explaining the cases where this is
wrong.


You should add some automated tests for both the visibility and display
cases, including the case of visibility:visible inside
visibility:hidden.

r=dbaron with that


At some point we should probably move this code away from using nsIDOM*
interfaces, but not in this patch.
Attachment #408707 - Flags: review?(dbaron) → review+
This part is "patch rev. 4" updated to trunk.
There's also a new problem that I'll address in part 2...

> >-#include "nsIDocumentEncoder.h"
> >-#include "nsIFrame.h"
> >-#include "nsISelectionController.h"
> 
> It seems like there's still direct use of these classes in the file...

There's no direct use of these classes in nsContentAreaDragDrop.cpp

> I don't think there's any need for |extern| here.

OK, removed. (my intent was to emphasize that it's in a
different file, rather than a forward declaration)
Attachment #408707 - Attachment is obsolete: true
Attached patch Patch rev. 5, part 2 (obsolete) — Splinter Review
See comment in patch.
Attachment #444522 - Flags: review?(dbaron)
Attached patch Patch rev. 5, part 3 (obsolete) — Splinter Review
This part fixes the remaining table collapsing cases (I think).

Also, I moved setting the SkipInvisibleContent bit to
nsCopySupport.cpp instead, it makes us handle text/unicode and
text/html in the same way with regards to invisible content;
After playing with it for a while I think this makes more sense.
Attachment #444526 - Flags: review?(dbaron)
Attached patch Patch rev. 5, part 4 (tests) (obsolete) — Splinter Review
Let me know if you have table collapsing cases you want included.

The patch pass TryServer unit tests on Linux and OSX, but there's
an error in this test on Windows (Selection.toString result have
\r\n where the expected string is \n), otherwise it passed also on
Windows.  I'll fix it before landing.
Blocks: 564967
Filed bug 564955 for problem 5 in Testcase #4.
Filed bug 564967 for the -moz-user-select:none case in Testcase #3.
All other tests works with patch rev. 5.
Attached patch Patch rev. 5, part 4, v2 (tests) (obsolete) — Splinter Review
.replace(/\r\n/g,"\n") fixed the test on Windows
Attachment #444532 - Attachment is obsolete: true
Comment on attachment 444522 [details] [diff] [review]
Patch rev. 5, part 2

Do we really want this?  In the examples I can think of, I wouldn't want this whitespace in the text representation of the selection, but I probably would want it in the HTML representation.

Either way, though, I think you need to put the visibility check *before* this check; otherwise all visibility:hidden elements will end up turning into large sequences of whitespace.

And given that, I don't think you actually need to check the node flags or TextIsOnlyWhitespace, since text nodes aren't stylable directly... though perhaps you should assert them.
Attachment #444522 - Flags: review?(dbaron) → review-
And please add tests for the cases in comment 91.
Blocks: 565508
Comment on attachment 444526 [details] [diff] [review]
Patch rev. 5, part 3

So this is a good bit of complexity, and extra additional storage, for a feature (visibility:collapse) that I think is basically broken as designed to begin with (given that 'visibility' inherits) and that's really quite hard to implement correctly.  And I still don't think it gets it right, since I don't see how it handles the interaction of collapsing and column/row spanning correctly.

I think we should just not bother handling collapsing rows/columns in this code; I don't think it's worth the energy to get it right or the additional code complexity even if we had a patch that did it right.
Attachment #444526 - Flags: review?(dbaron) → review-
(In reply to comment #93)
I'll skip the table collapsing handling for now and file a bug on that
separately.  We can postpone that until the table code does the collapsing
correctly.  Is there a table bug filed that I can block that work on?
Attachment #444522 - Attachment is obsolete: true
Attachment #445838 - Flags: review?(dbaron)
Attachment #444526 - Attachment is obsolete: true
Attachment #445839 - Flags: review?(dbaron)
Attachment #444800 - Attachment is obsolete: true
Comment on attachment 445838 [details] [diff] [review]
Patch rev. 6, part 2

r=dbaron, I suppose (with the caveats in the first paragraph of comment 91)

Maybe we should have a followup bug to further improve whitespace handling, though?  (Do we consider the CSS 'white-space' property when copying text?  I suppose if we considered it well we'd probably want to undo this change.)
Attachment #445838 - Flags: review?(dbaron) → review+
Comment on attachment 445839 [details] [diff] [review]
Patch rev. 6, part 3

r=dbaron, but why don't we want to set the flag in nsContentAreaDragDrop too?
Attachment #445839 - Flags: review?(dbaron) → review+
Whiteboard: [Hixie-P0][firebug-p2][needs-review] → [Hixie-P0][firebug-p2][needs-landing]
Blocks: 567362
(In reply to comment #99)
> r=dbaron, but why don't we want to set the flag in nsContentAreaDragDrop too?

nsContentAreaDragDrop now uses nsCopySupport::GetTransferableForSelection
which calls SelectionCopyHelper, so DnD now uses the same code copy-paste.
(see "part 1" of the patch)

(In reply to comment #98)
> Maybe we should have a followup bug to further improve whitespace handling

There is bug 116083.

I filed bug 567362 on fixing the remaining table visibility:collapse cases.
http://hg.mozilla.org/mozilla-central/rev/0c440c656ada
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: helpwanted
Resolution: --- → FIXED
Whiteboard: [Hixie-P0][firebug-p2][needs-landing] → [Hixie-P0][firebug-p2]
Target Milestone: --- → mozilla1.9.3a5
Depends on: 574596
Depends on: 602231
Depends on: 678800
Depends on: 703514
Depends on: 751778
(In reply to Leif Halvard Silli from comment #32)
> Created attachment 180245 [details]
> web page that shows the effect on «Find in this page» searching

I note that this bug is resolved as fixed. However my attachment 180245 [details] still fails to works.

Hence I opened bug 822458.
You need to log in before you can comment on or make changes to this bug.