14835 - No visual indication that images are selected

Status: VERIFIED FIXED

(Core :: DOM: Selection, enhancement, P3)

Description

[Michael Lowe]

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

Comment 1

[Kathleen :Brade]

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!

Comment 2

[Michael Lowe]

Attachment: Selection example

Comment 3

[Michael Lowe]

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.

Comment 4

[Eli Goldberg]

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.

Comment 5

[mjudge]

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.

Comment 6

[Michael Lowe]

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.

Comment 7

[rubydoo123]

waiting to hear back from the UI folks.

Comment 8

[Michael Lowe]

Attachment: Example 2

Comment 9

[Michael Lowe]

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.

Comment 10

[Michael Lowe]

Attachment: Mozilla example 2

Comment 11

[Michael Lowe]

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.

Comment 12

[Michael Lowe]

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.

Comment 13

[shuang (gone)]

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.

Comment 14

[Michael Lowe]

Attachment: Hilight proposal

Comment 15

[Michael Lowe]

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

[shuang (gone)]

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

[german]

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.

Comment 18

[Michael Lowe]

Attachment: Proposal 2

Comment 19

[Michael Lowe]

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.

Comment 20

[Michael Lowe]

Are we moving toward a decision on this?

Comment 21

[shuang (gone)]

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

Comment 22

[Lakespur Roca]

Thanks for your comments. You all have laid a good foundation for me. I'm
starting on this today.

Comment 23

[Matthew Thomas, usability weenie]

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

[Lakespur Roca]

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.

Comment 25

[Jim Roskind]

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.

Comment 26

[Mike]

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

Comment 27

[Blake Ross]

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

Comment 28

[Lakespur Roca]

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

[Blake Ross]

*SPAM*: Changing the QA contact of all open/resolved Selection bugs from 
[e-mail] to [e-mail].  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...

Comment 30

[Lakespur Roca]

Looks like this may be skinable rea signing to marlon for look.

Comment 31

[Matthew T (active 1999-2002)]

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

[marlon bishop]

while the UE team will continue make recommendations on this, this bug doesn't 
actually belong to themes. reassigning to owner of selection.

Comment 33

[rubydoo123]

moving to future

Comment 34

[Jesse Ruderman]

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

Comment 35

[Jesse Ruderman]

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

[Greg K.]

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

Comment 37

[Greg K.]

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

Comment 38

[Blake Ross]

changing selection qa to tpreston.

Comment 39

[Webmaster33]

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

Comment 40

[Mats Palmgren (inactive)]

*** Bug 163865 has been marked as a duplicate of this bug. ***

Comment 41

[mjudge]

ok this turns on image selection by default and it does an alpha blend on the 
image if its selected.

Comment 42

[mjudge]

Attachment: patch from layout\

this seems to work. apply from layout

Comment 43

[mjudge]

Attachment: 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.

Comment 44

[mjudge]

Attachment: idl file for layout/base/public

add this to the layout/base/public dir.

Comment 45

[mjudge]

Attachment: sample: new alpha blending selection

this is the alpha blending result of selection in geko

Comment 46

[mjudge]

Attachment: 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

[Jaime Rodriguez, Jr.]

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!

Comment 48

[Stuart Parmenter]

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

r=pavlov

Comment 49

[Jaime Rodriguez, Jr.]

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.

Comment 50

[Simon Fraser [no longer active]]

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

[Simon Fraser [no longer active]]

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?

Comment 52

[Christian :Biesinger (don't email me, ping me on IRC)]

>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.

Comment 53

[mjudge]

*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.

Comment 54

[Simon Fraser [no longer active]]

Can we see the final patch please?

Comment 55

[mjudge]

Attachment: new patch with fixes layout directory

new mac defines. not using prescontext. using the layout service instead
directly.

Comment 56

[mjudge]

Attachment: new idl without prescontext information

new idl to get rid of the prescontext passed into the service.

Comment 57

[Stuart Parmenter]

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

r=pavlov

Comment 58

[Conrad Carlen (not reading bugmail)]

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...

Comment 59

[mjudge]

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.

Comment 60

[mjudge]

Attachment: 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.

Comment 61

[mjudge]

Attachment: new idl no scriptable keyword

took out scriptable and removed noscript from the 2 methods.

Comment 62

[Simon Fraser [no longer active]]

+  }
+  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.

Comment 63

[mjudge]

Attachment: 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.

Comment 64

[mjudge]

Attachment: 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.

Comment 65

[Simon Fraser [no longer active]]

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

sr=sfraser, and bringing pavlov's r= forward

Comment 66

[Simon Fraser [no longer active]]

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

sr=sfraser, and bringing pavlov's r= forward

Comment 67

[Jaime Rodriguez, Jr.]

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.

Comment 68

[mjudge]

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

Comment 69

[Antony Mawer]

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

[Jaime Rodriguez, Jr.]

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!

Comment 71

[Jaime Rodriguez, Jr.]

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.

Comment 72

[mjudge]

marking it fixed to be veified

Comment 73

[Terri Preston]

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

Comment 74

[Randell Jesup [:jesup] (needinfo me)]

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

[Jaime Rodriguez, Jr.]

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

[Terri Preston]

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

[Terri Preston]

Attachment: Windows Selection of Images

Windows Selection

Comment 78

[Terri Preston]

Attachment: Mac's selection of images

Mac Selection

Comment 79

[mjudge]

Attachment: 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

[Terri Preston]

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

[Jaime Rodriguez, Jr.]

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

[Stuart Parmenter]

Attachment: set one row at a time

Comment 83

[Stuart Parmenter]

Attachment: set each row at a time reducing heap allocation

Comment 84

[Stuart Parmenter]

Attachment: set each row and free the right data

Comment 85

[Kathleen :Brade]

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).

Comment 86

[Kathleen :Brade]

reopening bug since this is not fixed

Comment 87

[Akkana Peck]

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

[Simon Fraser [no longer active]]

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.

Comment 89

[mjudge]

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.

Comment 90

[mjudge]

Attachment: nsFrame.cpp

this trims down the drawn area. hopefully this will trim off bad areas for the
mac alpha blend image drawing code.

Comment 91

[Jaime Rodriguez, Jr.]

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

[Akkana Peck]

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

Comment 93

[mjudge]

ok its all working now mac and linux.

Comment 94

[mjudge]

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..

Comment 95

[mjudge]

Attachment: 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

[Stuart Parmenter]

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

r=pavlov

Comment 97

[jag (Peter Annema)]

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

sr=jag

Comment 98

[mjudge]

oh and i tried it with rollovers and it works fine.  

Comment 99

[Jaime Rodriguez, Jr.]

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!

Comment 100

[Jaime Rodriguez, Jr.]

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

[kill this account]

images now "highlight" on my Redhat 7.2 system

Comment 102

[Yuying Long]

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

[Jennifer Kobayashi]

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

Comment 104

[marina]

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

[shrirang khanzode]

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

Comment 106

[Randell Jesup [:jesup] (needinfo me)]

Ok, seems to be pretty well verified.  mjudge, where's the branch version?

Comment 107

[Judson Valeski]

 please checkin to the MOZILLA_1_0_BRANCH branch. once there, remove the
"mozilla1.0.2+" keyword and add the "fixed1.0.2" keyword. 

Comment 108

[bobj]

Verified on US W2K and on Redhat 7.3

Comment 109

[Jaime Rodriguez, Jr.]

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.

Comment 110

[Simon Fraser [no longer active]]

Attachment: Patch to fix mac image scaling issue

Comment 111

[Jaime Rodriguez, Jr.]

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

[Jon Granrose]

we don't have builds.  looks like this broke the branch last night and no one
noticed.

Comment 113

[Jaime Rodriguez, Jr.]

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

[Terri Preston]

Yippie! Fixed win xp branch build 2002091908, linux branch 2002091906 and mac
branch 2002091905 keyword verified1.0.2 added (moving on to trunk builds)

Comment 115

[Terri Preston]

Fixed linux trunk build 2002091906, win xp trunk 2002091908 and mac os x trunk
2002091908 

Comment 116

[lchiang]

marking verified based on terri's comments for trunk verification.

Comment 117

[José Jeria]

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

Comment 118

[Nicolas Pioch]

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!!!