No visual indication that images are selected

VERIFIED FIXED in mozilla1.0.2

Status

()

Core
Selection
P3
enhancement
VERIFIED FIXED
18 years ago
15 years ago

People

(Reporter: Michael Lowe, Assigned: mjudge)

Tracking

({topembed+})

Trunk
mozilla1.0.2
topembed+
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2] [ETA 09/13] [FIXED ON TRUNK])

Attachments

(14 attachments, 11 obsolete attachments)

31.86 KB, image/gif
Details
17.92 KB, image/gif
Details
44.71 KB, image/gif
Details
33.28 KB, image/gif
Details
62.63 KB, image/gif
Details
217.18 KB, image/jpeg
Details
233.52 KB, image/jpeg
Details
21.72 KB, patch
Simon Fraser
: review+
Simon Fraser
: superreview+
Details | Diff | Splinter Review
1.20 KB, text/plain
Simon Fraser
: review+
Simon Fraser
: superreview+
Details
184.54 KB, image/jpeg
Details
93.71 KB, image/jpeg
Details
4.98 KB, patch
Details | Diff | Splinter Review
1.11 KB, patch
Stuart Parmenter
: review+
jag (Peter Annema)
: superreview+
Details | Diff | Splinter Review
1.47 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

18 years ago
It would be cool if Mozillla hilighted images which are part of the selection
the same way IE 5 does.

Updated

18 years ago
OS: Windows NT → All
Target Milestone: M15

Comment 1

18 years ago
Michael--how does IE5 do it?  Could you please describe so that those without IE5
(such as linux users?) will know how it should look?  Thanks!
(Reporter)

Comment 2

18 years ago
Created attachment 1858 [details]
Selection example
(Reporter)

Comment 3

18 years ago
See attached gif file for IE 5 example.    For comparison, NS4.6 does not
hilight selected images at all, and Mozilla currently draws selected images with
a 1 pixel blue outline, which can be difficult to see.

Updated

18 years ago
Summary: Image selection hilight → [Enh] Make image selection highlighting resemble IE 5 behavior

Comment 4

18 years ago
Clarified subject of this enhancement request.

Please note that the current image selection behavior is temporary. I'm not sure
what mjudge and/or german have in mind for the final behavior.
(Assignee)

Updated

18 years ago
Assignee: mjudge → beppe
Summary: [Enh] Make image selection highlighting resemble IE 5 behavior → Image selection hilight
(Assignee)

Comment 5

18 years ago
hmm well crap.  I am not sure what do do about this. this seems like a usability

issue.  I can change the outline to be bigger/different color, or we can try to

select the image the way ie5 does.  I may be biased but i dont really like the

way that looked. I like to still see the image unmolested when i select, but I

will do whatever usability decides.  I will not work on this until i get some

kind of definate response from whomever is doing usability though since I dont

have time to implement features only turned on by hidden preferences right now.

reassigning this to beth epperson since she is new Ender manager so she can

reassign this to the right person.
(Reporter)

Comment 6

18 years ago
I'm in favour of some consistency between the text selection hilight and the
images selection hilight.   For this reason, the image selection hilight should
at least be the same colour as the text selection hilight.   I also think the IE
method of image selection hilight looks more consistent with the text selection
hilight and for this reason I find it preferable over the current image outline
method.

Updated

18 years ago
Summary: Image selection hilight → [Enh] Make image selection hilighting resemble IE 5's behavior

Comment 7

18 years ago
waiting to hear back from the UI folks.
(Reporter)

Comment 8

18 years ago
Created attachment 1911 [details]
Example 2
(Reporter)

Updated

18 years ago
Summary: [Enh] Make image selection hilighting resemble IE 5's behavior → [Enh] What should be the image selection hilighting style?
(Reporter)

Comment 9

18 years ago
Another option is just to invert the image, as Word 97 seems to do it (see above
example).   I actually find this method preferable over the IE 5 method.
(Reporter)

Comment 10

18 years ago
Created attachment 1915 [details]
Mozilla example 2
(Reporter)

Comment 11

18 years ago
Also note that the selection hilight may need to be different in the editor
than what is is in the browser, since it may need to support additional
behavior, such as resizing the image by direct mouse manipulation.   Netscape
4.x Composer handles this by drawing 4 pixel inverted border around the image,
which can be dragged with the mouse to resize the image.
(Reporter)

Comment 12

18 years ago
I think the need to indicate additional image resizing behavior in the editor
using an outline hilight style is a good reason not to hilight images in the
browser using an outline style.   This reason supports my desire to hilight
images in the browser by inverting the whole image.

Updated

18 years ago
Assignee: beppe → shuang

Comment 13

18 years ago
This is a feedback UI item that no one owns it in UI group ( way too few
people). We may need to work on this with the rest of the feedback items such as
keyboard navigation etc. add lake on cc list since she is looking into other
items at this point.
I agree that the feedback for text and image selection should have some
consistance, but I am not sure whether IE's look is a good sample. It seems very
heavy to me. Inverting is another thing that looks like something is wrong (
looks like color palette shifting). The good thing about IE selection is that it
showed the entire image area. So we probably should make ours somewhat nicer and
easier to see. Please wait for some kind of spec (probably a screen shot) as a
sample to follow.
I will own this bug till we deliver the sample.
(Reporter)

Comment 14

18 years ago
Created attachment 1963 [details]
Hilight proposal
(Reporter)

Comment 15

18 years ago
Shuang: how about my latest proposal (see last attachment)?   It would use the
same hilight colour throught the text and image selection, whilst keeping the
image easy to see.

Comment 16

18 years ago
Michael, I saw the screen shot and I think it is probably good start. Do you
have any thoughts on the text selection? Another question is that how costly is
for engineers to do this or even possible since your screen shot has very nice
transparent colors. Third one will be: We need to have a proposal that works
with the lowest limitations, e.g. if someone has a 256 color monitor, or
set with gray scale,will this color treatment still work or will it looks very
bad ( this is why sometimes the line focusing works better). Thoughts?

Comment 17

18 years ago
I don't think it is a blocker type usability issue.
I would rather not like to see the image itself altered by dither patterns or
translucent overlaid colors (which probably will not work well on Mac and <inux
for now anyay). I'd rather like to see a border, but instead of talking extra
space and making the image wider, maybe we can paint the border on top of the
image. In order to make the full size of the image visible, the border would
only occur near the corners of the image like so:
___       ____
|             |
|             |


|             |
|___       ___|
My next preferred choice would be the dither pattern in the selection color,
which looks (while less nice) more like a selection and less like somebody
having altered the image.
(Reporter)

Comment 18

18 years ago
Created attachment 2012 [details]
Proposal 2
(Reporter)

Comment 19

18 years ago
German: some problems with borders are:

1. If the image is large, then only part of the image may be visible on-screen.
The image may be selected, but the user might not know this by what they can
currently see on the screen - the borders maybe outside the window viewport,
even though part of the image is visible.

2. With certain image and background colour combinations, it can be very hard to
see the borders, especially if the borders are thin.   Being colour bind myself,
I am particularly sensitive to these factors.

3. If image borders are used in the editor to indicate drag resizing facilities,
then it is not good to use the same style in the browser, or users could become
confused.

4. Image borders might look out of place when all the selected text is painting
in different style, without borders.

Regarding implementation difficulties on Unix or Mac, it may not be as difficult
as you expect - it should be much simpler than alpha blending, since you are
only proportionally mixing a solid color with the image.

Regarding dithering vs proportional colour mixing, I prefer the latter since it
leaves the image more visibly intact.

Shuang: Regarding text selection, I like the way 4.x does it - paint a solid
background colour behind the selected text, and then draw the selected text on
top of it.   Both the solid selection background colour and the selected text
colour should be specified through CSS files.

Regarding working on low colour monitors, see my latest attached proposal.   I
would sugest using a combination of German's proposed image borders along with
my proposal for proportional colour mixing.   On high bit-depth screens it would
use both styles combined (see bottom image in the attachment), while on lower
bit depth screens the code would degrade to just using Germans image borders
(see middle image in the attachment).

My proposal is probably a bit more work for the engineers to implement, but I
think it is worth it, for the reasons I've suggested above.
(Reporter)

Updated

18 years ago
Summary: [Enh] What should be the image selection hilighting style? → [Enh] What should be the selection hilighting style?
(Reporter)

Comment 20

18 years ago
Are we moving toward a decision on this?

Updated

18 years ago
Assignee: shuang → lake

Comment 21

18 years ago
re-assign it to lake. lake is working on a keyboard navigation spec.

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 22

18 years ago
Thanks for your comments. You all have laid a good foundation for me. I'm
starting on this today.
FWIW, I prefer IE's dithering style. It makes it as clear as possible that the
appearance of the image is due to selection, and not to funkiness on the part of
the artist.

The current border-style selection makes the image look like <a href=...><img
border="1" .../></a>, which is probably not a good idea -- especially if the
image is the only thing selected, because it looks like a link (or an artistic
effect). And this is always going to happen if the selection style is just a
border, no matter how thick it is.

Comment 24

18 years ago
The current thinking for image selection is heading in the direction of being
consistent with the text selection in color for highlighting. And have
redundancy for selection ques in that there should be a border and an entire
immage effect. These are being considered because a consistent look of selection
reenforces the meaning. And redundency of immage select ques makes it more clear
that an image or part of what the user percieves as an image is selected.
Additionally having the image border and entire image effect (possably
something like a transparent color effect) together further differentiates it
from how it would look if it had the focus. The details are still under
consideration.
(Reporter)

Updated

18 years ago
Summary: [Enh] What should be the selection hilighting style? → What should be the selection hilighting style?

Comment 25

18 years ago
Lots of great discussion... but nothing to suggest a change will make the M15 
stability checkpoint.  I'm pushing this to M16 to facilitate the M15 branch.
Target Milestone: M15 → M16

Comment 26

18 years ago
M16 has been out for a while now, these bugs target milestones need to be 
updated.

Updated

18 years ago
Target Milestone: M16 → M19

Comment 27

18 years ago
OK, so what's the deal with this one?

Comment 28

18 years ago
The actual highlighting style I believe, can be different for each skin as it is 
defined in XUL. Am I rigt? So the highlighting style would be different for 
different skins. However it would be nice to have highlighting for selection of 
pictures and a way to show secondary highlighting. I am coppying Marlon on this 
since he will be working on one of the skins. 

Comment 29

18 years ago
*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

Comment 30

18 years ago
Looks like this may be skinable rea signing to marlon for look.
Assignee: lake → marlon
Status: ASSIGNED → NEW

Comment 31

18 years ago
If it helps you understand this bug, add the word `default' in an appropriate 
place in the summary ... The fact that it's skinnable is really a red herring.

A few salient details:
* Just adding a border to the image would make it look like a link when the image
  wasn't a link, or make the selection almost invisible when the image was a
  link.
* Adding a border to the image also wouldn't work if the image was physically
  surrounded by other images, especially if one or more of those images was
  small.
* Doing anything which didn't affect the image itself wouldn't work well if the
  image was large (as border effects would appear mostly off-screen), or if the
  image featured stripes which happened to be roughly the same color as the
  selection color.
* Merely inverting the image wouldn't work well for rectangular (or
  mostly-rectangular) logos, as it would just look like the same logo with a
  different color scheme.
* Tinting the image wouldn't be very obvious, since many images on the Web are
  tinted anyway.
* IE's image selection style (50 % dithered selection color over the image) Just
  Makes Sense. Dithering is very unambiguous, because practically zero images on
  the Web use the same ordered dithering effect. And it is only `heavy' because
  Windows' text selection style is heavy. If you don't like the heaviness, change
  Windows' selection color (in the Display control panel) to a lighter color.

Comment 32

17 years ago
while the UE team will continue make recommendations on this, this bug doesn't 
actually belong to themes. reassigning to owner of selection.
Assignee: marlon → mjudge

Comment 33

17 years ago
moving to future
Target Milestone: M19 → Future

Comment 34

17 years ago
*** Bug 74029 has been marked as a duplicate of this bug. ***

Updated

17 years ago
Summary: What should be the selection hilighting style? → No visual indication that images are selected

Updated

17 years ago
Blocks: 96352

Comment 35

17 years ago
See also bug 96352, a similar problem in composer.  (In composer, there's a 
blue border around a selected image, making it look like a link.)

Comment 36

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

Comment 37

16 years ago
I marked the Composer part of this a duplicate of this bug since selection
behavior should be consistent between Navigator and Composer.

Comment 38

16 years ago
changing selection qa to tpreston.
QA Contact: blaker → tpreston

Comment 39

16 years ago
I place my vote to the following solution:
"My preferred choice would be the dither pattern in the selection color,
which looks more like a selection and less like somebody having altered the image."
It is showed correctly on "Hilight proposal" attachment image.

An additional options could be implemented for those, who don't want text &
images selected together.

Solutions:
1) Press CTRL + A: Select all texts on the page (without images)
2) Press CTRL + SHIFT + A: Select all texts & images on the page
3) When selecting with mouse drag, both text & image is selected, or just text,
depending on option 4)
4) Config option in Preferences:  Mouse selection selects: 
  a) images & text 
  b) only text
 ( default option is a) )
5) Config option in Preferences:  Color of image selection: 
  [ #CCCCCC ]  or  R: 128 G: 128 B: 128

I still very miss image selection feature from Mozilla v1.1 alpha.
Would be fine to have this feature implemented in Mozilla v1.2 release, as
suggested above.

Let me know opinions about the suggested solutions.

Thanks,
Webmaster33

Updated

16 years ago
Depends on: 21747
*** Bug 163865 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 41

15 years ago
ok this turns on image selection by default and it does an alpha blend on the 
image if its selected.
(Assignee)

Comment 42

15 years ago
Created attachment 98091 [details] [diff] [review]
patch from layout\

this seems to work. apply from layout
(Assignee)

Comment 43

15 years ago
Created attachment 98147 [details] [diff] [review]
patch from layout directory. with mac and unix fixes

ok added in ifdefs for image creation of mac and unix.	also forgot earlier to
add in new idl file to patch. I will post that idl next. my cvs foo isnt up to
adding new files yet.
Attachment #98091 - Attachment is obsolete: true
(Assignee)

Comment 44

15 years ago
Created attachment 98149 [details]
idl file for layout/base/public

add this to the layout/base/public dir.
(Assignee)

Comment 45

15 years ago
Created attachment 98176 [details]
sample: new alpha blending selection

this is the alpha blending result of selection in geko
(Assignee)

Comment 46

15 years ago
Created attachment 98177 [details]
sample: if current image selection was on in browser

this sample shows current selection behavior.  If we turn on image selection in
mail viewing or browsing it would look like this.

Comment 47

15 years ago
This is needed by a major embedding customer asap. Markeing as nsbeta1+ and
topembed+.

Mike: Can you layout our risk wrt to where we might regress or cause a stability
issue (if at all)? thanks!
Blocks: 154896
Keywords: mozilla1.0.2, nsbeta1+, topembed+
Whiteboard: [adt2] [ETA 09/11]

Comment 48

15 years ago
Comment on attachment 98147 [details] [diff] [review]
patch from layout directory. with mac and unix fixes

r=pavlov
Attachment #98147 - Flags: review+

Updated

15 years ago
Whiteboard: [adt2] [ETA 09/11] → [adt2] [ETA 09/12] [needs sr=]

Comment 49

15 years ago
kin/simon: could one of you provide a sr= for mike's patch? we need to get it
landed on the trunk and verified asap, as it is needed by a major embedding
customer.
Keywords: mozilla1.2
Target Milestone: Future → mozilla1.0.2

Comment 50

15 years ago
First pass comments:

+
+// frame image selection drawing service implementation
+class SelectionImageService : public nsISelectionImageService
+{
+public:
+  SelectionImageService();
+  virtual ~SelectionImageService();
+  NS_DECL_ISUPPORTS
+  NS_DECL_NSISELECTIONIMAGESERVICE
+private:
+  nsCOMPtr<imgIContainer> mContainer;
+};
+

Is this really a service in the XPCOM sense? If it is, then it has to be able to
behave as a singleton, and should be stateless. It's not clear from this code
whether those conditions are met.

+NS_IMPL_ADDREF(SelectionImageService)
+NS_IMPL_RELEASE(SelectionImageService)
+NS_IMPL_QUERY_INTERFACE1(SelectionImageService, nsISelectionImageService)

You can do all this with 1 macro (NS_IMPL...).

+#ifdef XP_MAC
+#define SEL_TRIPPLESIZE 4
+#else
+#define SEL_TRIPPLESIZE 3
+#endif

triple, or tripple? XP_MAC needs to be defined(XP_MAC) || defined(XP_MACOSX).
And, anyway, why is Mac different? A comment would be nice, (Mac uses RGBA or
something).

+NS_IMETHODIMP
+SelectionImageService::CreateImage(nsIPresContext*  aPresContext)

I'd like to see a comment saying what this method is actually doing.

+        nscolor selectionTextColor = NS_RGB(255, 255, 255);
+	      nsILookAndFeel* look = nsnull;
+	      if (NS_SUCCEEDED(aPresContext->GetLookAndFeel(&look)) && look) {

You can use do_GetService to get the nsILookAndFeel, which means that you can
lose the 'aPresContext' param to this method.

+	        look->GetColor(nsILookAndFeel::eColor_TextSelectBackground,
selectionTextColor);
+	        NS_RELEASE(look);
+	      }

Spacing weirdness here (tabs?)

+#ifdef XP_MAC

needs to be 
#if defined(XP_MAC) || defined(XP_MACOSX)


+        image->SetImageData(data,SEL_IMAGE_TOTAL_SIZE,
SEL_IMAGE_TOTAL_SIZE-SEL_WIDTH_BYTES);
+       
image->SetAlphaData(alpha,SEL_IMAGE_ALPHA_SIZE,SEL_IMAGE_ALPHA_SIZE-SEL_IMAGE_WIDTH);

Is it OK to call SetImageData and SetAlphaData with pointers to stack-based
data? Does imglib copy the data?


+    nsCOMPtr<nsISelectionImageService> imageService;
+    imageService = do_GetService(kSelectionImageService, &result);

If this service is used in the same file as its implementation, do we really
need the XPCOM overhead? Why not just have a global, which is cleaned up by some
shutdown method?

This could also make painting on pages with lots of selected images very slow.
Have you tested the performance impact?

Index: html/base/src/nsImageFrame.cpp

This file seems to contain some unrequired changes.

Comment 51

15 years ago
Thinking about it some more, I have the following points/questions:
1. Tiling a transparent image over the selected image seems like an expensive
   way to do something that should be possible by just drawing the image with
   a certain transfer mode in the first place.

2. What does the result look like when the user is in 32000 colors mode,
   and 256 color mode?
>Is it OK to call SetImageData and SetAlphaData with pointers to stack-based
>data? Does imglib copy the data?

yes, imglib does copy the data.
(Assignee)

Comment 53

15 years ago
*yes this is a service in that you create one by calling do_GetService.  there
is only 1 of these and following calls retreive the same one.

*I can replace the macros for addref,release and QI. no big deal

*I wanted someone with mac know how to look at these IFDEFS to make sure I had
the right ones.  I will replace #ifdef XP_MAC with defined(XP_MAC) ||
defined(XP_MACOSX)

*Mac is different because it wants RGBX  The last byte is completely irrelevant.
 I will put a comment in there. I would hope no one would touch this code
without knowlege of formats on different platforms to begin with though! :)

*I used the prescontext's look and feel because I wasnt sure if there could be
more than one.  I will replace the parameter.  One thing this does enforce
however is that you cant try to create and image until you have a prescontext
anyway but thats not that important.

*yes the image lib copies it. if not it would surely have crashed when I was
testing. (I think the fact that I cant create the buffer and pass it off is
kinda dumb but thats image lib)

*globals are terrible ideas.  we have a method for creating a 1 and only 1 of an
XPCOM implementation and thats a service.  the overhead is not that great. just
1 entry in a hash table.  I think the way here is the best way and the most
interchangeable if we want to make the method for image creation (i.e. hash
patterns ect) change in the future.

*256 color and 32000 color mode works like a champ on windows.

*The drawing of images will of course become slower when they are selected.  I
have done testing on my 650 mz machine and it looks fine. I will test some more.
I would appreciate some tests on mac.
Status: NEW → ASSIGNED

Comment 54

15 years ago
Can we see the final patch please?
(Assignee)

Comment 55

15 years ago
Created attachment 98552 [details] [diff] [review]
new patch with fixes layout directory

new mac defines. not using prescontext. using the layout service instead
directly.
Attachment #98147 - Attachment is obsolete: true
(Assignee)

Comment 56

15 years ago
Created attachment 98553 [details]
new idl without prescontext information

new idl to get rid of the prescontext passed into the service.
Attachment #98149 - Attachment is obsolete: true

Comment 57

15 years ago
Comment on attachment 98552 [details] [diff] [review]
new patch with fixes layout directory

r=pavlov
Attachment #98552 - Flags: review+
I think having a service to hold this image data is OK.

Other things:

[scriptable, uuid(f6f68e3c-f078-4235-bf71-53d180c37d26)]

Instead of declaring the iface as scriptable and then each method "noscript",
why not just lose the "scriptable" here?

interface nsISelectionImageService : nsISupports
{
	/**
	* Create the image used to alpha blend selection
	*/
	[noscript] void createImage();


Since createImage() is done implicitly by getImage(), why does it need to be in
the public API?

+#if defined(XP_MAC) || defined(XP_MACOSX)
+          data[i] = 0
+          data[i+1] = NS_GET_R(selectionTextColor);
+          data[i+2] = NS_GET_G(selectionTextColor);
+          data[i+3] = NS_GET_B(selectionTextColor);
+#endif

On the PowerPC, you pay a penalty for accessing data that isn't long-aligned.
Instead of stuffing each byte individually and paying 4 times, shift these bytes
into a long and write the long in one shot. Granted, this is a small image, but...
(Assignee)

Comment 59

15 years ago
I dont need the noscript anymore.  I will remove noscript instead from the
methods. thanks for seeing that I forgot.

The reason you can call create image seperately is to force the service to
refresh the image.  I will need to do this to handle when/IF the selection color
changes during the program execution.  If you change the color the old image is
no good.  I will post a patch with your suggestions in it(most of them) and also
add the CreateImage call to the prescontext method that gets called when the
system colors change.
(Assignee)

Comment 60

15 years ago
Created attachment 98584 [details] [diff] [review]
sparkling patch of layout/

This leaves the data[i] data[i+1] alone for mac simply because it would simply
confuse the code that only is run for the first row of a 32 wide image.  This
adds the context check for changing of the system colors.  Works great. the
images change their color as well.  This shows how the service was a very good
idea in this case.

I also removed the scriptable like you suggested.  there was no need for it
after thinking about it.

so besides the MAC specific long copy, I think this answers all your concerns.
Attachment #98552 - Attachment is obsolete: true
(Assignee)

Comment 61

15 years ago
Created attachment 98585 [details]
new idl no scriptable keyword

took out scriptable and removed noscript from the 2 methods.
Attachment #98553 - Attachment is obsolete: true

Comment 62

15 years ago
+  }
+  nsCOMPtr<nsISelectionImageService> imageService;
+  nsresult result;
+  imageService = do_GetService(kSelectionImageService, &result);
+  if (NS_SUCCEEDED(result) && imageService)
+  {
+    imageService->CreateImage();
   }

Why is it better to make the image here than the first time the service is
requested (in an Init() method), or on the first call to getImage()? If the user
never selects an image (i.e. on 95% of web pages ever loaded) then you're
wasting resources.

Creating the image in an Init method would enable you to lose the 'createImage'
in the interface, which, as conrad pointed out, is redundant.

+#if defined(XP_MAC) || defined(XP_MACOSX)
+#define SEL_TRIPPLESIZE 4
+#else
+#define SEL_TRIPPLESIZE 3
+#endif

"tripple" is still mis-spelled, and there is still no comment about why mac is
different.

+        nscolor selectionTextColor = NS_RGB(255, 255, 255);
+	      nsCOMPtr<nsILookAndFeel> look;
+        look = do_GetService(kLookAndFeelCID,&result);
+	      if (NS_SUCCEEDED(result) && look) {
+	        look->GetColor(nsILookAndFeel::eColor_TextSelectBackground,
selectionTextColor);
+	      }
+        //its better to temporarily go after heap than put big data on stack

Tabs here still.

Index: html/base/src/nsImageFrame.cpp

There are still bogus changes in this file.
(Assignee)

Comment 63

15 years ago
Created attachment 98621 [details] [diff] [review]
patch for /layout

fixes spelling and tabs. also adds in concept of disabled selection to the
"getImage" call.  ReCreateImage is gone and replaced with "Reset" on the idl
file.  This is to simply null out the images currently stored.	Then the next
time an image is asked for with GetImage, the images needed will be recreated.
Attachment #98584 - Attachment is obsolete: true
(Assignee)

Comment 64

15 years ago
Created attachment 98622 [details]
idl file for selection image service

this replaces ReCreateImage with just a simple Reset.  This does no work other
than free up the current images stored.
Attachment #98585 - Attachment is obsolete: true

Comment 65

15 years ago
Comment on attachment 98621 [details] [diff] [review]
patch for /layout

sr=sfraser, and bringing pavlov's r= forward
Attachment #98621 - Flags: superreview+
Attachment #98621 - Flags: review+

Comment 66

15 years ago
Comment on attachment 98622 [details]
idl file for selection image service

sr=sfraser, and bringing pavlov's r= forward
Attachment #98622 - Flags: superreview+
Attachment #98622 - Flags: review+

Comment 67

15 years ago
thanks for the sr= simon. let's get this one landed on the trunk, if drivers
will let us. alternatively, we should get a private build ready for QA to verify
the fix.
Keywords: approval, edt1.0.2
Whiteboard: [adt2] [ETA 09/12] [needs sr=] → [adt2] [ETA 09/12] [Needs EDT & Mozilla approval]
(Assignee)

Comment 68

15 years ago
its in on the trunk. for about a couple hours now.  leave the bug open for
checkin to 1.02?

Comment 69

15 years ago
As a thought, how does this cope with things like javascript rollovers and
animated GIFs, where the selected image might change after it's selected?

From the screenshot, it does look _much_ nicer than the IE method of dithering.
Good work!

Comment 70

15 years ago
tpreston: can you pls verify this as fixed in today's trunk builds. if you can't
get to it today , lemme know (as it is a blocker for 1.0.2), and we can have
someone in iQA verify it. thanks!
Whiteboard: [adt2] [ETA 09/12] [Needs EDT & Mozilla approval] → [adt2] [ETA 09/12] [Needs EDT & Drivers approval for the 1.0 branch]

Comment 71

15 years ago
Mike: Go ahead and resolve this as fixed, so that QA knows to verify it on the
trunk. We use keyword foo, to manage notification for 1.0 branch checkins.
Whiteboard: [adt2] [ETA 09/12] [Needs EDT & Drivers approval for the 1.0 branch] → [adt2] [ETA 09/12] [FIXED ON TRUNK] [Needs EDT & Drivers approval for the 1.0 branch]
(Assignee)

Comment 72

15 years ago
marking it fixed to be veified
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 73

15 years ago
Looks great using win XP mozilla build 2002091108 (trunk build failed to
install) Doesn't work so great on mac and haven't gotten to linux but I'm
marking this verified for win and will open new bug if needed per conversation
with jaime
Status: RESOLVED → VERIFIED
OS: All → Windows XP
Given Terri's comments (doesn't look so good on Mac, haven't gotten to Linux)
and the lack of time on trunk, I'm skeptical that it's ready for the branch. 
Also, there's been no answer about javascript rollovers, or images changed by JS
after being selected.  It certainly seems like there's possibility of regression
here.

Comment 75

15 years ago
teri: can you pls write up the details for the Mac & Linux issue, or site the
bug? also, can you check around for js rollover regressions? thanks!

Randell/Jud: Would it be ok to check in the fix for Windows, and fix Mac/Linux
at a later time? I ask, because the major embedding customer who needs this fix
is on Windows. Alternatively, do you have any suggestions on how we could
accomodate the embedding customer wrt this issue?

Comment 76

15 years ago
Okay, on Mac OS X.2 (Jaguar) trunk build 20020901108) and linux nightly
2002091108, the highlighted portion includes all text before and after any
images but does not show hightlighting on the image itself, Windows does this
nicely and it is very clear that the image is highlighted

Comment 77

15 years ago
Created attachment 98754 [details]
Windows Selection of Images

Windows Selection

Comment 78

15 years ago
Created attachment 98755 [details]
Mac's selection of images

Mac Selection
(Assignee)

Comment 79

15 years ago
Created attachment 98756 [details] [diff] [review]
fix for mac and unix

this should fix mac and unix. the problem was the windows bitmaps are bottom up
and so the offsets for the start of the data have to point to the bototm of the
buffer while mac and unix point to the top.

Comment 80

15 years ago
I do not see any JS rollover regressions (Again, using win XP mozilla build
2002091108, Mac OS X.2 (Jaguar) trunk build 20020901108 and linux nightly
2002091108)

Comment 81

15 years ago
Mike: Can you (or, someone) apply and test your patch with a current build on
Linux and Mac, before we take this on the trunk? We are running a little low on
time, and we need this to be good to go for the 1.0 brnach tomorrow. thanks!

Comment 82

15 years ago
Created attachment 98768 [details] [diff] [review]
set one row at a time

Comment 83

15 years ago
Created attachment 98772 [details] [diff] [review]
set each row at a time reducing heap allocation
Attachment #98756 - Attachment is obsolete: true
Attachment #98768 - Attachment is obsolete: true

Comment 84

15 years ago
Created attachment 98779 [details] [diff] [review]
set each row and free the right data
Attachment #98772 - Attachment is obsolete: true

Comment 85

15 years ago
Comment on attachment 98779 [details] [diff] [review]
set each row and free the right data

This patch doesn't work on Mac.  I see problems when I do this:
 in browser, go to cnn.com
 select little picture on right side (and text or not) [be careful not to
trigger its link]
 scroll down so you can't see selection
 scroll back up so you can see selection
 notice that the image has a "shadow" to its right and below it

This is really ugly; it won't be removed without removing selection and forcing
a refresh of the window.

You can also minimize/maximize and see this same effect as well as many other
things which force the area to repaint (resize, change focus, etc).
Attachment #98779 - Flags: needs-work+

Comment 86

15 years ago
reopening bug since this is not fixed
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Updated

15 years ago
OS: Windows XP → All

Comment 87

15 years ago
The latest patch has the same "snow" problem on linux that I reported on IRC
from the patch this morning: if you drag-select across an image (including text
on one side or the other), then mouse out of the window and back in (with
pointer focus), snow (random noise) appears on the lower parts (anywhere from
the lower half to 4/5) of the image.

Comment 88

15 years ago
Confirming problems on Mac. The issue is that we don't always clip the tiling of
the alpha image correctly to the size of the image. I'm not sure why not.
(Assignee)

Comment 89

15 years ago
ok stuart is workin on getting a mac and unix build right now.  I suspect the
unix problem should be defeated soon. the mac one not clipping is very odd to me.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 90

15 years ago
Created attachment 98822 [details] [diff] [review]
nsFrame.cpp

this trims down the drawn area. hopefully this will trim off bad areas for the
mac alpha blend image drawing code.
Attachment #98779 - Attachment is obsolete: true

Comment 91

15 years ago
Who can review the latest patch. We really need to get this landed on  the trunk
tonight for all platforms, if we are gonna meet the deadline for a major
embedding customer. pls make best efforts. thanks!

Comment 92

15 years ago
I can report that the latest patch (98822) does cure the snow problem on linux,
and works correctly for all images I tested.
(Assignee)

Comment 93

15 years ago
ok its all working now mac and linux.
(Assignee)

Comment 94

15 years ago
simple fix on mac (like 2 lines) I will post what I checked in tomorrow. the
nsFrame change is also simple.  I still recommend this for the 1.0 branch and I
hope the activity here doesnt scare anyone. it really is a straight forward
image alpha blend tiling on top of selected images.  just a couple quirks to
clip the mac properly and feed the image lines in one at a time (helps unix
out).  I am going home i am pretty tired..
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED
(Assignee)

Comment 95

15 years ago
Created attachment 98869 [details] [diff] [review]
fix for possible leak

fix to patch previously applied patch. possible memory leak if we are allready
out of memory. but might as well fix this.

Comment 96

15 years ago
Comment on attachment 98869 [details] [diff] [review]
fix for possible leak

r=pavlov
Attachment #98869 - Flags: review+

Comment 97

15 years ago
Comment on attachment 98869 [details] [diff] [review]
fix for possible leak

sr=jag
Attachment #98869 - Flags: superreview+
(Assignee)

Comment 98

15 years ago
oh and i tried it with rollovers and it works fine.  

Comment 99

15 years ago
tpreston: can you pls verify this as fixed in today's trunk builds. if you can't
get to it today , lemme know (as it is a blocker for 1.0.2), and we can have
someone in iQA verify it. thanks!
cc'ing shrir, jennifer and yuying for additional testing on this fix. thanks!

fyi - from verbal conversation with mjudge. all patches are in for this issue.
nothing else needs to land on the trunk. he will generate a combined patch for
the 1.0 branch, and attach it.

Comment 101

15 years ago
images now "highlight" on my Redhat 7.2 system

Comment 102

15 years ago
Verified images can be highlighted in 09-12 trunk build with:
WinXP-SC, linux RH7.2-JA, Mac 10.1.5 and 9.1-JA.

Comment 103

15 years ago
Verified this is fixed on 2002-09-12-branch with:
Redhat 7.0 
Mac OS X 

Comment 104

15 years ago
verified the fix: images can be selected when Select all with 2002-09-12-08
trunk build on Windows, MacOS9.1 and Linux 

Comment 105

15 years ago
I too verified this on winNT, OSX, OS9.2, Linux7.1 and redhat 7.3 with 0912 
trunk builds. Things look fine.

Updated

15 years ago
Keywords: edt1.0.2 → edt1.0.2+
Ok, seems to be pretty well verified.  mjudge, where's the branch version?

Updated

15 years ago
Whiteboard: [adt2] [ETA 09/12] [FIXED ON TRUNK] [Needs EDT & Drivers approval for the 1.0 branch] → [adt2] [ETA 09/13] [FIXED ON TRUNK]

Comment 107

15 years ago
 please checkin to the MOZILLA_1_0_BRANCH branch. once there, remove the
"mozilla1.0.2+" keyword and add the "fixed1.0.2" keyword. 
Keywords: mozilla1.0.2 → mozilla1.0.2+

Comment 108

15 years ago
Verified on US W2K and on Redhat 7.3
gonna go ahead and mark this one as verified for the trunk, per Comment #105
From shrirang khanzode, since terri is ooto until 09/17.
Status: RESOLVED → VERIFIED

Comment 110

15 years ago
Created attachment 98976 [details] [diff] [review]
Patch to fix mac image scaling issue
(Assignee)

Updated

15 years ago
Keywords: mozilla1.0.2+ → fixed1.0.2
shrir: can you pls verify this as fixed in tomorrow's 1.0 branch builds? once,
verified, pls replace "fixed1.0.2" with "verified1.0.2". thanks!

Comment 112

15 years ago
we don't have builds.  looks like this broke the branch last night and no one
noticed.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
tpreston: can you pls verify this as fixed on the 1.0 branch and trunk builds?
once, verified, pls replace "fixed1.0.2" with "verified1.0.2". thanks!

This should be fixed in all places now, right?

Comment 114

15 years ago
Yippie! Fixed win xp branch build 2002091908, linux branch 2002091906 and mac
branch 2002091905 keyword verified1.0.2 added (moving on to trunk builds)
Keywords: fixed1.0.2 → verified1.0.2

Comment 115

15 years ago
Fixed linux trunk build 2002091906, win xp trunk 2002091908 and mac os x trunk
2002091908 
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED

Comment 116

15 years ago
marking verified based on terri's comments for trunk verification.
Status: RESOLVED → VERIFIED

Comment 117

15 years ago
Image-submit-buttons are selected in the wrong way. See bug 173239.

Comment 118

15 years ago
does anyone care about bug 178152
selecting on a webpage immediately causes core dump
if selection includes an image?

(all Unix flavors, 8-bit displays)

Makes Mozilla 1.2b utterly useless

Please at least back out patch RFE/ bug 14835 which causes this havoc!!!
You need to log in before you can comment on or make changes to this bug.